[users] add module#2
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds a users domain and JWT-based auth: DB migration, Bun models/repository, Argon2id password handling, users service, JWT service/config, Fiber auth and admin handlers, JWT middleware, OpenAPI updates, request examples, and FX module wiring for jwt and users. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthHandler as Auth Handler
participant UsersSvc as Users Service
participant Repo as Users Repo
participant DB
participant JWTSvc as JWT Service
Client->>AuthHandler: POST /auth/login {email,password}
AuthHandler->>UsersSvc: Login(email,password)
UsersSvc->>Repo: GetByEmail(email)
Repo->>DB: SELECT FROM users WHERE email=?
DB-->>Repo: row / no row
alt user found
Repo-->>UsersSvc: *UserWithPasswordHash
UsersSvc->>UsersSvc: verify password (Argon2id)
alt verified & active
UsersSvc-->>AuthHandler: *User
AuthHandler->>JWTSvc: GenerateToken(user)
JWTSvc-->>AuthHandler: token
AuthHandler-->>Client: 200 {access_token, user}
else invalid or not active
UsersSvc-->>AuthHandler: ErrInvalidCredential / ErrNotActive
AuthHandler-->>Client: 401 / 403
end
else not found
Repo-->>UsersSvc: ErrNotFound
UsersSvc-->>AuthHandler: ErrInvalidCredential
AuthHandler-->>Client: 401
end
sequenceDiagram
participant Client
participant AuthHandler as Auth Handler
participant UsersSvc as Users Service
participant Repo as Users Repo
participant DB
Client->>AuthHandler: POST /auth/register {email,password}
AuthHandler->>UsersSvc: Register(input)
UsersSvc->>UsersSvc: hash password (Argon2id)
UsersSvc->>Repo: Create(input, hash)
Repo->>DB: INSERT INTO users (...)
alt unique violation
DB-->>Repo: constraint error
Repo-->>UsersSvc: ErrEmailAlreadyUsed
UsersSvc-->>AuthHandler: error
AuthHandler-->>Client: 409
else success
DB-->>Repo: inserted row
Repo-->>UsersSvc: *User
UsersSvc-->>AuthHandler: *User
AuthHandler-->>Client: 201 {user}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
internal/db/migrations/20260408120000_users.sql (1)
8-13: Add an index onstatusfor list/filter performance.Given the admin listing/status management flow, filtering by
statuswill benefit from an index.Proposed migration tweak
CREATE TABLE IF NOT EXISTS `users` ( @@ PRIMARY KEY (`id`), - UNIQUE KEY `idx_users_email` (`email`) + UNIQUE KEY `idx_users_email` (`email`), + KEY `idx_users_status` (`status`) ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/migrations/20260408120000_users.sql` around lines 8 - 13, Add a non-unique index on the users.status column to improve list/filter performance: update the migration in internal/db/migrations/20260408120000_users.sql for the users table by adding an index named idx_users_status on the status column (e.g., add KEY `idx_users_status` (`status`) in the CREATE TABLE or a separate CREATE INDEX for table `users`) so admin status queries use the index.internal/users/repository.go (1)
61-78: Consider adding pagination support.The method returns all matching users without limit. For large user bases, this could cause performance and memory issues. Consider adding
limitandoffsetparameters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/repository.go` around lines 61 - 78, The ListByStatus method currently returns all matching users and needs pagination: update the Repository.ListByStatus signature to accept pagination parameters (e.g., limit int, offset int) and validate they are non-negative (and cap limit if desired), then apply them to the SQL builder via query = query.Limit(limit).Offset(offset) before query.Scan(ctx). Ensure callers of ListByStatus are updated to pass limit/offset and keep conversion from userModel to domain User (toDomainUser) unchanged; handle zero or negative limit as either defaulting to a safe max or returning an error according to your policy.internal/users/handler.go (1)
72-91: Validate status query parameter and consider adding pagination.The
statusquery parameter is directly cast toStatuswithout validation. Invalid values will be passed to the database query. Additionally, returning all users without pagination could cause performance issues at scale.Proposed fix for status validation
func (h *Handler) list(c *fiber.Ctx) error { statusQuery := c.Query("status") var status *Status if statusQuery != "" { parsed := Status(statusQuery) + if !parsed.IsValid() { + return fiber.NewError(fiber.StatusBadRequest, "invalid status value") + } status = &parsed }This assumes a
Status.IsValid()method exists or is added to validate against allowed enum values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/handler.go` around lines 72 - 91, Validate the status query and add basic pagination in Handler.list: parse c.Query("status") into Status and call Status.IsValid() (or implement it) and return a 400 error for invalid values instead of passing them to h.service.List; parse pagination params (e.g., "page" and "limit" or "offset" and "limit") with sane defaults and bounds, convert to integers, and pass the pagination values into the service call (update h.service.List signature or add a new ListWithPagination method) so the DB query uses LIMIT/OFFSET; ensure the JSON response includes items and total (use a total returned from the service if available) and avoid returning the entire dataset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/users/dto.go`:
- Around line 3-34: The file has formatting issues around the DTO definitions
(e.g., RegisterRequest, LoginRequest, ChangePasswordRequest,
AdminUpdateUserStatusRequest, UserResponse, LoginResponse); run the Go formatter
across the repository or on internal/users/dto.go (for example use go fmt ./...
or gofmt -w internal/users/dto.go) to fix spacing/formatting so the
golines/static checks pass, then re-run the linter/CI.
In `@internal/users/handler.go`:
- Line 69: The LoginResponse currently returns a hardcoded token
("todo-jwt-token") which triggers gosec G101; replace this with real JWT
generation by implementing and calling a token builder (e.g., generateJWT or
auth.GenerateToken) from the login handler in internal/users/handler.go that
signs a JWT with your configured secret/key, includes necessary claims (user ID,
expiry, issued-at), uses a secure signing method (e.g., RS256/HS256 per config),
handles and logs generation errors, and returns the signed token string in
LoginResponse instead of the placeholder, returning an appropriate HTTP error if
token creation fails.
- Around line 24-31: The Register method currently exposes admin-only and
sensitive routes without protection; update Handler.Register to attach
authentication middleware (e.g., authMiddleware) to the /users group or
individual routes, add role-based authorization (e.g., requireRole("admin")) to
g.Get("", h.list) and g.Patch("/:id/status", h.updateStatus), and change the
g.Post("/:id/change-password", h.changePassword) handler to enforce that the
authenticated user ID matches the :id path param or that the caller has the
admin role (e.g., authorizeSelfOrRole("admin")) before allowing a password
change; implement/reuse middleware helpers named authMiddleware, requireRole,
and authorizeSelfOrRole and apply them in Handler.Register and/or inside the
registerd handlers (register, login remain public).
- Around line 12-15: The struct definition for Handler mixes an embedded field
and a regular field without a blank line which triggers
embeddedstructfieldcheck; update the Handler struct by inserting a single empty
line between the embedded field "handler.Base" and the regular field "service
*Service" so the embedded field is separated from non-embedded fields.
- Around line 93-113: The function updateStatus shadows the variable err in
multiple places causing lint failures; replace the short redeclarations with
single assignments to the existing err variable: use err =
strconv.ParseUint(...) converted to assign to a distinct typed variable for id
(e.g., parsedID) or declare id and err together before parsing, then change "if
err := h.BodyParserValidator(...)" to "if err = h.BodyParserValidator(c, &req)"
and change "if err := h.service.UpdateStatus(...)" to "if err =
h.service.UpdateStatus(c.Context(), id, req.Status)"; keep the existing
ErrUserNotFound check and return logic unchanged.
- Around line 115-138: The changePassword handler is shadowing the outer err
variable with short declarations at the BodyParserValidator call and the
service.ChangePassword call; update those to use reassignment instead of :=
(e.g., replace "if err := h.BodyParserValidator(...)" and "if err :=
h.service.ChangePassword(...)" with "if err = h.BodyParserValidator(...)" and
"if err = h.service.ChangePassword(...)" or rename the inner variables) so the
single err declared after ParseUint is reused; keep the same error checks
against ErrUserNotFound and ErrInvalidCredential and retain the existing
fiber.NewError returns and final SendStatus.
In `@internal/users/repository.go`:
- Around line 122-128: Replace the fragile string-matching helper
isDuplicateKeyError by calling the canonical db.IsUniqueViolation(err) from
internal/db/errors.go inside the Create() flow (and any other callers) and
remove the isDuplicateKeyError function; update Create() to import/use the db
package and propagate the db.IsUniqueViolation(err) result where uniqueness
checks are performed so behavior remains the same but uses error-code based
detection.
- Around line 21-33: The Create method in Repository uses a fragile
isDuplicateKeyError string match; replace that check with the existing
db.IsUniqueViolation(err) helper to detect MySQL unique-violation errors
reliably. In the Create function (Repository.Create) change the conditional from
isDuplicateKeyError(err) to db.IsUniqueViolation(err) and ensure the package
that defines IsUniqueViolation is imported so the error branch returns
ErrEmailAlreadyUsed as before.
In `@internal/users/service.go`:
- Around line 19-37: In Register, avoid shadowing the earlier err from
bcrypt.GenerateFromPassword by not using := on the s.repo.Create call; reuse the
existing err variable (assign with =) so the bcrypt error isn't shadowed—locate
the call to s.repo.Create(ctx, &user) inside func (s *Service) Register and
change the err declaration there to reuse the outer err variable and handle it
as before.
In `@internal/users/storage_model.go`:
- Around line 35-43: The userModel literal returned in toStorageUser is missing
the embedded BaseModel which triggers the exhaustruct lint; update the return to
initialize the embedded BaseModel field (e.g., set BaseModel:
storage.BaseModel{ID: u.ID, CreatedAt: u.CreatedAt, UpdatedAt: u.UpdatedAt} or
the appropriate BaseModel type used in this package) and then remove the
duplicated ID/CreatedAt/UpdatedAt fields from the top-level so the userModel
struct is fully and explicitly populated.
---
Nitpick comments:
In `@internal/db/migrations/20260408120000_users.sql`:
- Around line 8-13: Add a non-unique index on the users.status column to improve
list/filter performance: update the migration in
internal/db/migrations/20260408120000_users.sql for the users table by adding an
index named idx_users_status on the status column (e.g., add KEY
`idx_users_status` (`status`) in the CREATE TABLE or a separate CREATE INDEX for
table `users`) so admin status queries use the index.
In `@internal/users/handler.go`:
- Around line 72-91: Validate the status query and add basic pagination in
Handler.list: parse c.Query("status") into Status and call Status.IsValid() (or
implement it) and return a 400 error for invalid values instead of passing them
to h.service.List; parse pagination params (e.g., "page" and "limit" or "offset"
and "limit") with sane defaults and bounds, convert to integers, and pass the
pagination values into the service call (update h.service.List signature or add
a new ListWithPagination method) so the DB query uses LIMIT/OFFSET; ensure the
JSON response includes items and total (use a total returned from the service if
available) and avoid returning the entire dataset.
In `@internal/users/repository.go`:
- Around line 61-78: The ListByStatus method currently returns all matching
users and needs pagination: update the Repository.ListByStatus signature to
accept pagination parameters (e.g., limit int, offset int) and validate they are
non-negative (and cap limit if desired), then apply them to the SQL builder via
query = query.Limit(limit).Offset(offset) before query.Scan(ctx). Ensure callers
of ListByStatus are updated to pass limit/offset and keep conversion from
userModel to domain User (toDomainUser) unchanged; handle zero or negative limit
as either defaulting to a safe max or returning an error according to your
policy.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: add9bba0-eecc-45b8-91ed-938db026bb3b
📒 Files selected for processing (11)
internal/app.gointernal/db/migrations/20260408120000_users.sqlinternal/users/doc.gointernal/users/domain.gointernal/users/dto.gointernal/users/errors.gointernal/users/handler.gointernal/users/module.gointernal/users/repository.gointernal/users/service.gointernal/users/storage_model.go
capcom6
force-pushed
the
codex/design-users-module
branch
from
0137793 to
7fa606a
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (6)
internal/config/config.go (1)
65-69: Consider extracting magic number to a named constant.The static analysis tool flags
15as a magic number. Whiletime.Minute * 15is readable, extracting it to a named constant would improve maintainability and satisfy the linter.♻️ Suggested fix
+const defaultAccessTTLMinutes = 15 + func Default() Config { //nolint:gosec // default values return Config{ // ... other fields ... JWT: jwtConfig{ Secret: "secret", - AccessTTL: time.Minute * 15, + AccessTTL: time.Minute * defaultAccessTTLMinutes, Issuer: "bitissues.dev", }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 65 - 69, The AccessTTL value uses a magic number (time.Minute * 15); extract this duration into a named constant (e.g., defaultAccessTokenTTL or accessTokenTTL) and replace the literal in the jwtConfig initialization so JWT: jwtConfig{ Secret: "secret", AccessTTL: <constant>, Issuer: "bitissues.dev" } uses the constant; add the constant near other config defaults or top of the package with a clear name and type time.Duration to satisfy the linter and improve maintainability.internal/server/admin/users/dto.go (1)
46-55: Consider consolidating duplicate DTOs.This
UserResponseandtoUserResponseare nearly identical toUserResponseDTOandtoUserResponseDTOininternal/server/auth/dto.go. If the response structures are intentionally the same, consider extracting a shared type to reduce duplication.However, keeping them separate is acceptable if you anticipate the admin and auth responses diverging in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/admin/users/dto.go` around lines 46 - 55, The admin DTO UserResponse and converter toUserResponse duplicate the auth DTO UserResponseDTO and toUserResponseDTO; if they are intended to be identical now, extract a shared type (e.g., SharedUserResponse) and a single converter (e.g., toSharedUserResponse) used by both packages, or alternatively export the existing DTO from one package and reuse it in the other; update references in functions that currently call toUserResponse/toUserResponseDTO to use the consolidated type and converter (or import the exported type) to remove duplication while preserving namespaced behavior if you expect future divergence.internal/server/auth/dto.go (2)
44-50: Same maximum password length concern applies here.For consistency with
RegisterRequest, add a maximum length constraint toNewPassword.Proposed fix
type ChangePasswordRequest struct { OldPassword string `json:"old_password" validate:"required"` - NewPassword string `json:"new_password" validate:"required,min=8"` + NewPassword string `json:"new_password" validate:"required,min=8,max=72"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth/dto.go` around lines 44 - 50, The ChangePasswordRequest.NewPassword validate tag is missing a maximum length constraint; update the struct tag on NewPassword in ChangePasswordRequest to include the same max length rule used by RegisterRequest (e.g., add max=XXX to the validate tag) so password length limits are consistent across registration and change-password flows.
9-15: Consider adding maximum password length validation.While
min=8is appropriate, there's no maximum length constraint. bcrypt has a 72-byte limit and will silently truncate longer passwords, which could lead to unexpected behavior. Additionally, extremely long passwords without a limit could cause performance issues during hashing.Consider adding
max=72(or a reasonable upper bound like 128) to prevent truncation issues and potential DoS.Proposed fix
type RegisterRequest struct { - Email string `json:"email" validate:"required,email"` - Password string `json:"password" validate:"required,min=8"` + Email string `json:"email" validate:"required,email"` + Password string `json:"password" validate:"required,min=8,max=72"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth/dto.go` around lines 9 - 15, The RegisterRequest struct's Password field lacks a maximum length validation; update the struct tag on RegisterRequest.Password to include a max constraint (e.g., add validate:"required,min=8,max=72" or use max=128 if you prefer a higher bound) so the validator enforces an upper limit to avoid bcrypt truncation and hashing performance issues.internal/server/admin/users/handler.go (2)
156-163: Potential race condition between update and fetch.There's a small window between
Update(line 145) andGetByID(line 157) where another request could modify or delete the user. While unlikely in practice, ifGetByIDreturnsErrNotFound, theerrorsHandlerwill map it to 404, which could be confusing after a successful update.Consider returning the updated data directly from the
Updatemethod if atomicity is important, or accept this as acceptable MVP behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/admin/users/handler.go` around lines 156 - 163, There’s a race between calling usersSvc.Update and then usersSvc.GetByID which can yield ErrNotFound after a successful Update; change usersSvc.Update (or an overload) to return the updated user object atomically (update signature in usersSvc.Update -> return (*User, error)) and use that returned user in the handler instead of calling GetByID, or alternatively catch ErrNotFound after GetByID and treat it as success by constructing the response from the Update result; update the handler code that currently calls Update and GetByID to use the new Update return value (or to handle ErrNotFound) so the response is consistent and avoids the race.
83-91: Consider using query parser withAdminUserFilterstruct.The
AdminUserFilterstruct hasdefaulttags but they're not utilized here. Manual parsing works but is more verbose. Usingc.QueryParser(&filter)would be more concise and leverage the struct's default values.Additionally, the condition
o >= 0at line 89 is always true sinceQueryIntreturns 0 for invalid/missing values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/admin/users/handler.go` around lines 83 - 91, Replace the manual QueryInt parsing with the AdminUserFilter struct and c.QueryParser so the struct's `default` tags are honored: create an AdminUserFilter variable, call c.QueryParser(&filter) to populate Limit/Offset (and other filters) and then use filter.Limit and filter.Offset instead of the manual limit/offset variables; remove the current QueryInt branches (and the redundant o >= 0 check) so defaults from the struct are used consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/jwt/service.go`:
- Around line 43-47: In GenerateToken, the call to token.SignedString is passing
s.config.Secret as a string causing an invalid key type at runtime; change it to
pass a byte slice like in ValidateToken by converting s.config.Secret with
[]byte(s.config.Secret) so token.SignedString receives a []byte key and the
signing succeeds.
In `@internal/server/admin/users/dto.go`:
- Around line 19-25: The UpdateUserRequest enums are only for docs and
handleUpdate must validate incoming values; in handleUpdate (the HTTP handler
that reads UpdateUserRequest) check if req.Status != nil and call
users.IsValidStatus(*req.Status) and if req.Role != nil call
users.IsValidRole(*req.Role), returning a 400/validation error when either is
invalid before calling the service update method; this mirrors the validation
performed in handleList and prevents invalid enum strings from reaching the
service layer.
In `@internal/server/auth/handler.go`:
- Around line 110-118: The handler currently ignores req.OldPassword allowing
password changes without verification; update the flow so handleChangePassword
calls usersSvc.ChangePassword with (ctx, userID, req.OldPassword,
req.NewPassword) (or similar) and ensure the users.Service.ChangePassword
signature and implementation verify the old password before updating; map a
failed old-password verification to an appropriate error (e.g., return
fiber.StatusUnauthorized or a clear validation error) and update any other
callers/tests to the new signature. Ensure ChangePasswordRequest (and any DTOs)
include OldPassword usage and that error wrapping preserves context for logging.
In `@internal/server/docs/docs.go`:
- Around line 191-194: Fix the malformed Swagger `@Success` annotation in
internal/server/auth/handler.go by replacing the incorrect line that produces
`"No Content\"\t\"Password changed successfully"` (the existing `// `@Success` 204
"No Content" "Password changed successfully"` style) with a correctly formatted
annotation — either a plain message form or the typed form (e.g., `// `@Success`
204 "Password changed successfully"` or `// `@Success` 204 {string} string
"Password changed successfully"`). Update the annotation associated with the
password-change handler so the generated Swagger responses map the 204
description correctly.
In `@internal/server/middlewares/jwtauth/jwtauth.go`:
- Around line 20-26: The documentation for New in jwtauth.go incorrectly
documents the type for "user_id"; update the comment to match the actual type
used (jwt.Claims.UserID is an int64) so change the comment line that says
`"user_id": uint64` to `"user_id": int64` and keep the rest unchanged; refer to
the New function and jwt.Claims.UserID to locate the relevant comment.
In `@internal/server/module.go`:
- Around line 31-35: The users.NewHandler is returning *users.Handler which is
not being registered as handler.Handler in the FX group, so only auth.NewHandler
(which returns handler.Handler) ends up in the []handler.Handler consumer;
update the fx.Provide fx.Annotate for users.NewHandler to include
fx.As(new(handler.Handler)) so FX registers its result as the interface type,
e.g. change fx.Annotate(users.NewHandler, fx.ResultTags(`group:"handlers"`)) to
include fx.As to cast to handler.Handler; keep the auth.NewHandler annotation
as-is so both join the same []handler.Handler group consumed by the route
registration.
In `@internal/users/models.go`:
- Around line 33-37: The code currently persists the caller-supplied role
(u.Role) when creating a new user; change the creation path in
internal/users/models.go to force the role to RoleUser (i.e., set Role:
RoleUser) instead of using u.Role so Service.Register/ UserInput cannot escalate
privileges — leave StatusPending and other fields unchanged and ensure any role
changes are performed only via the admin update flow.
In `@internal/users/password.go`:
- Around line 67-85: Validate the parsed Argon2 parameters (memory, iterations,
parallelism) and the computed hash length before narrowing conversions in the
verification path in internal/users/password.go: ensure parallelism <= 255 and
non-zero, iterations and memory are within safe non-zero bounds your application
expects, and len(hash) fits into uint32 and is >0; if any check fails return
ErrInvalidCredential (as done elsewhere) instead of proceeding to argon2.IDKey,
so that the call argon2.IDKey([]byte(password), salt, iterations, memory,
uint8(parallelism), uint32(len(hash))) only runs with validated, non-wrapping
values.
In `@internal/users/repository.go`:
- Around line 100-104: The update statements that modify user fields (e.g., the
NewUpdate() call that targets Model((*userModel)(nil)) and sets "password_hash =
?") must also set the UpdatedAt timestamp so bunfx.TimedModel stays in sync;
modify those update paths (password update, status change, role change — the
NewUpdate() blocks around the password_hash and the other mutation blocks) to
Set the "updated_at" column to the current time (use time.Now()/UTC as your
project convention) alongside the existing Set calls so toDomain() returns an
up-to-date UpdatedAt.
- Around line 123-130: The Update repository currently executes the built query
even when no SET clauses are added (both update.Status and update.Role are nil);
add validation in the Update function (in internal/users/repository.go) after
building the query and before calling query.Exec(ctx) to detect that no fields
were set and return a clear error instead of executing an empty UPDATE.
Implement this by checking whether any update field was applied (e.g., track a
boolean or count when you call query.Set for Status/Role) and return a sentinel
error (e.g., ErrNoUpdateFields) or fmt.Errorf("no fields to update") from the
Update method when none are set. Ensure callers can detect this error and do not
proceed to call query.Exec when validation fails.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 65-69: The AccessTTL value uses a magic number (time.Minute * 15);
extract this duration into a named constant (e.g., defaultAccessTokenTTL or
accessTokenTTL) and replace the literal in the jwtConfig initialization so JWT:
jwtConfig{ Secret: "secret", AccessTTL: <constant>, Issuer: "bitissues.dev" }
uses the constant; add the constant near other config defaults or top of the
package with a clear name and type time.Duration to satisfy the linter and
improve maintainability.
In `@internal/server/admin/users/dto.go`:
- Around line 46-55: The admin DTO UserResponse and converter toUserResponse
duplicate the auth DTO UserResponseDTO and toUserResponseDTO; if they are
intended to be identical now, extract a shared type (e.g., SharedUserResponse)
and a single converter (e.g., toSharedUserResponse) used by both packages, or
alternatively export the existing DTO from one package and reuse it in the
other; update references in functions that currently call
toUserResponse/toUserResponseDTO to use the consolidated type and converter (or
import the exported type) to remove duplication while preserving namespaced
behavior if you expect future divergence.
In `@internal/server/admin/users/handler.go`:
- Around line 156-163: There’s a race between calling usersSvc.Update and then
usersSvc.GetByID which can yield ErrNotFound after a successful Update; change
usersSvc.Update (or an overload) to return the updated user object atomically
(update signature in usersSvc.Update -> return (*User, error)) and use that
returned user in the handler instead of calling GetByID, or alternatively catch
ErrNotFound after GetByID and treat it as success by constructing the response
from the Update result; update the handler code that currently calls Update and
GetByID to use the new Update return value (or to handle ErrNotFound) so the
response is consistent and avoids the race.
- Around line 83-91: Replace the manual QueryInt parsing with the
AdminUserFilter struct and c.QueryParser so the struct's `default` tags are
honored: create an AdminUserFilter variable, call c.QueryParser(&filter) to
populate Limit/Offset (and other filters) and then use filter.Limit and
filter.Offset instead of the manual limit/offset variables; remove the current
QueryInt branches (and the redundant o >= 0 check) so defaults from the struct
are used consistently.
In `@internal/server/auth/dto.go`:
- Around line 44-50: The ChangePasswordRequest.NewPassword validate tag is
missing a maximum length constraint; update the struct tag on NewPassword in
ChangePasswordRequest to include the same max length rule used by
RegisterRequest (e.g., add max=XXX to the validate tag) so password length
limits are consistent across registration and change-password flows.
- Around line 9-15: The RegisterRequest struct's Password field lacks a maximum
length validation; update the struct tag on RegisterRequest.Password to include
a max constraint (e.g., add validate:"required,min=8,max=72" or use max=128 if
you prefer a higher bound) so the validator enforces an upper limit to avoid
bcrypt truncation and hashing performance issues.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 545ca410-38e2-4b8a-bbac-e580002748da
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
go.modinternal/app.gointernal/config/config.gointernal/config/module.gointernal/db/migrations/20260408120000_users.sqlinternal/jwt/config.gointernal/jwt/doc.gointernal/jwt/domain.gointernal/jwt/errors.gointernal/jwt/module.gointernal/jwt/service.gointernal/server/admin/users/dto.gointernal/server/admin/users/handler.gointernal/server/auth/dto.gointernal/server/auth/handler.gointernal/server/docs/docs.gointernal/server/middlewares/jwtauth/jwtauth.gointernal/server/module.gointernal/users/doc.gointernal/users/domain.gointernal/users/errors.gointernal/users/models.gointernal/users/module.gointernal/users/password.gointernal/users/repository.gointernal/users/service.gorequests.http
✅ Files skipped from review due to trivial changes (4)
- internal/users/doc.go
- requests.http
- internal/db/migrations/20260408120000_users.sql
- internal/users/module.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/app.go
- internal/users/errors.go
- internal/users/domain.go
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
internal/users/repository.go (1)
95-97: WrapCounterrors with repository context.Line 96 returns raw DB errors directly; wrapping them (like other methods do) improves observability and consistency.
♻️ Proposed refinement
count, err := query.Count(ctx) - return int64(count), err + if err != nil { + return 0, fmt.Errorf("failed to count users: %w", err) + } + return int64(count), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/repository.go` around lines 95 - 97, The raw DB error from query.Count(ctx) is returned directly; wrap it with repository context to match other methods. Change the return to convert count and, if err != nil, return 0 and wrap the error (e.g., fmt.Errorf("users repository: Count failed: %w", err) or errors.Wrap) so callers get repository-scoped error context while still returning int64(count) on success; refer to query.Count(ctx) and the surrounding method in internal/users/repository.go when applying this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/jwt/service.go`:
- Around line 27-40: GenerateToken currently dereferences the user pointer
(e.g., user.ID, user.Role, user.Status) without a nil check; add a nil guard at
the start of Service.GenerateToken that returns a clear error when user is nil
(do not panic), e.g., validate user != nil and return an appropriate error value
before constructing Claims/RegisteredClaims so the rest of the function (Claims,
RegisteredClaims, use of strconv.FormatInt, uuid.New()) can assume a non-nil
user.
In `@internal/server/admin/users/dto.go`:
- Around line 15-16: Fiber's QueryParser doesn't apply `default` tags, so the
DTO fields Limit and Offset (Limit and Offset in the users DTO) will be zero;
update the handler that parses the query into this DTO (the user list handler
that calls c.QueryParser / QueryParser) to explicitly apply defaults after
parsing: if dto.Limit <= 0 set dto.Limit = 20 (and optionally enforce a max),
and ensure dto.Offset is >= 0 (set dto.Offset = 0 if negative) before using them
for pagination.
In `@internal/server/docs/docs.go`:
- Around line 140-159: Add authentication failure responses to the OpenAPI
responses block for the secured admin PATCH endpoint: update the responses map
(the same block showing "200", "400", "404") to include "401" (Unauthorized) and
"403" (Forbidden) entries that reference the existing error schema (e.g.,
fiberfx.ErrorResponse) and add a brief description; also ensure the operation
includes the BearerAuth security requirement so the generated docs reflect that
the route is protected.
In `@internal/users/repository.go`:
- Around line 134-139: The Update method currently executes the SQL via
query.Exec(ctx) but doesn't check the affected row count, so an update against a
non-existent id returns nil; modify the Update implementation to capture the
Exec result (e.g., res, err := query.Exec(ctx)), check res.RowsAffected() and if
it returns 0 return the package's ErrNotFound, otherwise return any Exec error
or nil; reference the Update function, query.Exec call, and ErrNotFound symbol
when making this change.
---
Nitpick comments:
In `@internal/users/repository.go`:
- Around line 95-97: The raw DB error from query.Count(ctx) is returned
directly; wrap it with repository context to match other methods. Change the
return to convert count and, if err != nil, return 0 and wrap the error (e.g.,
fmt.Errorf("users repository: Count failed: %w", err) or errors.Wrap) so callers
get repository-scoped error context while still returning int64(count) on
success; refer to query.Count(ctx) and the surrounding method in
internal/users/repository.go when applying this change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 903ae547-d56a-4ff7-87a8-f3ede885cbec
📒 Files selected for processing (13)
internal/config/config.gointernal/jwt/service.gointernal/server/admin/users/dto.gointernal/server/admin/users/handler.gointernal/server/auth/dto.gointernal/server/auth/handler.gointernal/server/docs/docs.gointernal/server/middlewares/jwtauth/jwtauth.gointernal/users/domain.gointernal/users/password.gointernal/users/repository.gointernal/users/service.gorequests.http
✅ Files skipped from review due to trivial changes (1)
- requests.http
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/server/middlewares/jwtauth/jwtauth.go
- internal/config/config.go
- internal/users/password.go
- internal/server/auth/handler.go
- internal/server/admin/users/handler.go
- internal/server/auth/dto.go
- internal/users/domain.go
- internal/users/service.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/users/password.go (1)
60-98:⚠️ Potential issue | 🟠 MajorValidate all parsed Argon2 fields before recomputing the hash.
Line 86 still allows malformed values through (
memory == 0,iterations == 0,len(hash) == 0, and missing header validation). That can causeargon2.IDKeymisuse and can let an empty decoded hash pass verification.🔧 Proposed fix
func verifyPasswordArgon2id(password, encodedHash string) error { const valsCount = 6 // Parse the encoded hash: $argon2id$v=19$m=19456,t=2,p=1$<salt>$<hash> vals := strings.Split(encodedHash, "$") if len(vals) != valsCount { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } + if vals[1] != "argon2id" || vals[2] != fmt.Sprintf("v=%d", argon2.Version) { + return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) + } var memory, iterations, parallelism uint32 _, err := fmt.Sscanf(vals[3], "m=%d,t=%d,p=%d", &memory, &iterations, ¶llelism) if err != nil { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } @@ hash, err := base64.StdEncoding.DecodeString(vals[5]) if err != nil { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } - if parallelism == 0 || parallelism > math.MaxUint8 || len(hash) > math.MaxUint16 { + if memory == 0 || iterations == 0 || + parallelism == 0 || parallelism > math.MaxUint8 || + len(salt) == 0 || len(hash) == 0 || + uint64(len(hash)) > uint64(math.MaxUint32) { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } @@ iterations, memory, uint8(parallelism), - uint32(len(hash)), //nolint:gosec // checked abpve + uint32(len(hash)), //nolint:gosec // checked above )#!/bin/bash # Read-only verification for guard coverage and Argon2 API expectations. rg -n -C3 'func verifyPasswordArgon2id|vals\[1\]|vals\[2\]|memory == 0|iterations == 0|len\(hash\) == 0|argon2\.IDKey' internal/users/password.go go doc golang.org/x/crypto/argon2.IDKey | sed -n '1,160p'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/password.go` around lines 60 - 98, The Argon2 verify path (e.g., verifyPasswordArgon2id) currently parses vals but doesn't validate critical fields before calling argon2.IDKey; add explicit checks that vals[1] == "argon2id" and vals[2] contains a valid version matching argon2.Version, and validate memory != 0, iterations != 0, parallelism != 0, and len(hash) != 0 (and that len(hash) <= math.MaxUint16 and parallelism <= math.MaxUint8) and return ErrInvalidCredential on failure; only after those guards call argon2.IDKey with the validated parameters.
🧹 Nitpick comments (2)
internal/server/admin/users/dto.go (1)
15-16: Keep pagination defaults single-sourced to avoid drift.
Limit/Offsetdefaults are duplicated in tags and indefaultAdminUserFilter(). Consider centralizing the runtime values in constants and reusing them in the factory.♻️ Suggested refactor
+const ( + defaultUsersLimit = 20 + defaultUsersOffset = 0 +) + func defaultAdminUserFilter() AdminUserFilter { //nolint:mnd // default values return AdminUserFilter{ Status: nil, Role: nil, - Limit: 20, - Offset: 0, + Limit: defaultUsersLimit, + Offset: defaultUsersOffset, } }Also applies to: 47-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/admin/users/dto.go` around lines 15 - 16, Define canonical defaults as constants (e.g. DefaultAdminUserLimit, DefaultAdminUserOffset) and use those constants in defaultAdminUserFilter() and any other runtime places that apply (references in the factory/constructors), then update the struct tag default values on the Limit and Offset fields in the DTO (and the other occurrences noted around lines 47-55) so they match these constants; this keeps the runtime code single-sourced while ensuring the tags are synchronized with the new constants.internal/server/auth/handler.go (1)
131-144: Prefer stable client error messages over wrapped internal strings.Using
err.Error()in mapped responses can expose internal wrapper context (e.g.,"failed to login user: ..."). Return canonical public messages per error type instead.♻️ Proposed refactor
switch { case errors.Is(err, users.ErrEmailAlreadyUsed): - return fiber.NewError(fiber.StatusConflict, err.Error()) + return fiber.NewError(fiber.StatusConflict, users.ErrEmailAlreadyUsed.Error()) case errors.Is(err, users.ErrNotFound): - return fiber.NewError(fiber.StatusNotFound, err.Error()) + return fiber.NewError(fiber.StatusNotFound, users.ErrNotFound.Error()) case errors.Is(err, users.ErrInvalidCredential): - return fiber.NewError(fiber.StatusUnauthorized, err.Error()) + return fiber.NewError(fiber.StatusUnauthorized, users.ErrInvalidCredential.Error()) case errors.Is(err, users.ErrNotActive): - return fiber.NewError(fiber.StatusForbidden, err.Error()) + return fiber.NewError(fiber.StatusForbidden, users.ErrNotActive.Error()) case errors.Is(err, jwt.ErrInvalidToken): - return fiber.NewError(fiber.StatusUnauthorized, err.Error()) + return fiber.NewError(fiber.StatusUnauthorized, "invalid token") case errors.Is(err, jwt.ErrExpiredToken): - return fiber.NewError(fiber.StatusUnauthorized, err.Error()) + return fiber.NewError(fiber.StatusUnauthorized, "token expired")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth/handler.go` around lines 131 - 144, The switch in auth handler that maps internal errors to HTTP responses (the errors.Is checks for users.ErrEmailAlreadyUsed, users.ErrNotFound, users.ErrInvalidCredential, users.ErrNotActive, jwt.ErrInvalidToken, jwt.ErrExpiredToken) should return stable, public-facing messages instead of err.Error(); update each fiber.NewError call in that switch to use a canonical string (e.g., "email already in use", "user not found", "invalid credentials", "account not active", "invalid token", "token expired") so internal wrapped context is not leaked—prefer simple constant messages (or package-level message constants) and keep the same status codes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/config/config.go`:
- Around line 65-67: The default JWT.Secret value ("secret") in the jwtConfig
initialization is insecure and shorter than the required 32 bytes; replace it so
the application uses a secure key source (e.g., read from an environment
variable like JWT_SECRET or generate a 32+ byte random value at startup) and
ensure jwtConfig.Secret meets the 32-byte minimum enforced by the JWT validator.
Update the jwtConfig/JWT initialization to validate presence/length and fail
fast with a clear error if the env-provided secret is missing or too short,
referencing the jwtConfig struct and the JWT.Secret field when making the
change.
---
Duplicate comments:
In `@internal/users/password.go`:
- Around line 60-98: The Argon2 verify path (e.g., verifyPasswordArgon2id)
currently parses vals but doesn't validate critical fields before calling
argon2.IDKey; add explicit checks that vals[1] == "argon2id" and vals[2]
contains a valid version matching argon2.Version, and validate memory != 0,
iterations != 0, parallelism != 0, and len(hash) != 0 (and that len(hash) <=
math.MaxUint16 and parallelism <= math.MaxUint8) and return ErrInvalidCredential
on failure; only after those guards call argon2.IDKey with the validated
parameters.
---
Nitpick comments:
In `@internal/server/admin/users/dto.go`:
- Around line 15-16: Define canonical defaults as constants (e.g.
DefaultAdminUserLimit, DefaultAdminUserOffset) and use those constants in
defaultAdminUserFilter() and any other runtime places that apply (references in
the factory/constructors), then update the struct tag default values on the
Limit and Offset fields in the DTO (and the other occurrences noted around lines
47-55) so they match these constants; this keeps the runtime code single-sourced
while ensuring the tags are synchronized with the new constants.
In `@internal/server/auth/handler.go`:
- Around line 131-144: The switch in auth handler that maps internal errors to
HTTP responses (the errors.Is checks for users.ErrEmailAlreadyUsed,
users.ErrNotFound, users.ErrInvalidCredential, users.ErrNotActive,
jwt.ErrInvalidToken, jwt.ErrExpiredToken) should return stable, public-facing
messages instead of err.Error(); update each fiber.NewError call in that switch
to use a canonical string (e.g., "email already in use", "user not found",
"invalid credentials", "account not active", "invalid token", "token expired")
so internal wrapped context is not leaked—prefer simple constant messages (or
package-level message constants) and keep the same status codes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cd36dc4-1ad8-40c5-9ed6-aae4bad9d1ae
📒 Files selected for processing (7)
internal/config/config.gointernal/server/admin/users/dto.gointernal/server/admin/users/handler.gointernal/server/auth/handler.gointernal/server/docs/docs.gointernal/users/password.gointernal/users/repository.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/server/docs/docs.go
- internal/server/admin/users/handler.go
- internal/users/repository.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/admin/users/dto.go`:
- Around line 61-69: The toUserResponse function dereferences its *users.User
input unconditionally which will panic if u is nil; update toUserResponse to
guard against nil by returning an empty UserResponse (or zero-value with only
safe fields) when u == nil, e.g., check `if u == nil { return UserResponse{} }`
at the top, then populate fields from u for non-nil inputs so callers of
toUserResponse cannot trigger a nil-pointer panic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57420249-7f35-48bf-b77a-36ffe57f2a30
📒 Files selected for processing (1)
internal/server/admin/users/dto.go
capcom6
force-pushed
the
codex/design-users-module
branch
from
9a3273d to
5662e83
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/users/password.go (1)
70-98:⚠️ Potential issue | 🔴 CriticalHarden Argon2 parameter validation before calling
IDKey.At Line 86/Line 97, validation is still incomplete: zero-length decoded hash/salt and zero
memory/iterationsare not rejected, and the cast target isuint32but the guard usesmath.MaxUint16. Please reject malformed values before recomputing.🔧 Proposed fix
var memory, iterations, parallelism uint32 _, err := fmt.Sscanf(vals[3], "m=%d,t=%d,p=%d", &memory, &iterations, ¶llelism) if err != nil { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } + if memory == 0 || iterations == 0 || parallelism == 0 || parallelism > math.MaxUint8 { + return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) + } salt, err := base64.StdEncoding.DecodeString(vals[4]) if err != nil { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } hash, err := base64.StdEncoding.DecodeString(vals[5]) if err != nil { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } - - if parallelism == 0 || parallelism > math.MaxUint8 || len(hash) > math.MaxUint16 { + if len(salt) == 0 || len(hash) == 0 || len(hash) > math.MaxUint32 { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } // Compute hash of the provided password with same parameters computedHash := argon2.IDKey( []byte(password), salt, iterations, memory, uint8(parallelism), - uint32(len(hash)), //nolint:gosec // checked abpve + uint32(len(hash)), //nolint:gosec // checked above )#!/bin/bash # Read-only verification of current guards around Argon2 verification inputs. rg -n -C2 'memory == 0|iterations == 0|len\(salt\) == 0|len\(hash\) == 0|math\.MaxUint16|math\.MaxUint32|uint32\(len\(hash\)\)' internal/users/password.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/password.go` around lines 70 - 98, The current guards before calling argon2.IDKey are incomplete: add explicit rejects for memory == 0, iterations == 0, len(salt) == 0 and len(hash) == 0, and ensure len(hash) fits into the uint32 cast used for argon2.IDKey (i.e., validate that uint64(len(hash)) <= max uint32) instead of the existing math.MaxUint16 check; update the conditional that currently references memory, iterations, parallelism, salt and hash (variables memory, iterations, parallelism, salt, hash) so it returns ErrInvalidCredential for any of these malformed values before computing computedHash with argon2.IDKey.
🧹 Nitpick comments (4)
internal/db/migrations/20260408120000_users.sql (1)
8-8: Consider adding an index onstatusfor efficient filtering.The repository includes a
ListByStatusmethod (referenced in PR objectives), but there's no index on thestatuscolumn. As the user table grows, queries filtering by status will require full table scans.📊 Proposed index addition
Add an index on
statusto optimize status-based queries:`created_at` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, `updated_at` DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, PRIMARY KEY (`id`), - UNIQUE KEY `idx_users_email` (`email`) + UNIQUE KEY `idx_users_email` (`email`), + KEY `idx_users_status` (`status`) ) ENGINE = InnoDB DEFAULT CHARSET = utf8mb4 COLLATE = utf8mb4_unicode_ci;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/migrations/20260408120000_users.sql` at line 8, The users table lacks an index on the status column, causing ListByStatus (and any status-filter queries) to do full table scans; update the migration in internal/db/migrations/20260408120000_users.sql to add an index on `status` (e.g., CREATE INDEX or ALTER TABLE ... ADD INDEX `idx_users_status` (`status`)) so ListByStatus and similar methods can use the index for efficient filtering.internal/users/password.go (1)
61-68: Validate hash metadata tokens explicitly.After Line 61, consider rejecting hashes where
vals[1]is notargon2idorvals[2]is not av=token before parsing params. It tightens malformed-input handling.💡 Suggested adjustment
vals := strings.Split(encodedHash, "$") if len(vals) != valsCount { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) } + if vals[1] != "argon2id" || !strings.HasPrefix(vals[2], "v=") { + return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/password.go` around lines 61 - 68, Ensure the hash metadata tokens are validated before parsing: after checking len(vals) != valsCount, explicitly verify that vals[1] == "argon2id" and that vals[2] starts with "v=" (and optionally parse the version number) and return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) if they do not match; update the logic around the vals, valsCount, and ErrInvalidCredential checks in internal/users/password.go so malformed tokens are rejected early (e.g., in the function handling password hash parsing).internal/server/admin/users/handler.go (2)
143-160: Mirror JWT error translation in the admin error handler.This group is also wrapped by
jwtauth.New(h.jwtSvc)on Line 36, but the switch only mapsusers.*errors. Addjwt.ErrInvalidTokenandjwt.ErrExpiredTokenhere too so admin routes keep the same 401 behavior asinternal/server/auth/handler.go.Possible patch
switch { case errors.Is(err, users.ErrEmailAlreadyUsed): return fiber.NewError(fiber.StatusConflict, err.Error()) case errors.Is(err, users.ErrNotFound): return fiber.NewError(fiber.StatusNotFound, err.Error()) case errors.Is(err, users.ErrInvalidCredential): return fiber.NewError(fiber.StatusUnauthorized, err.Error()) case errors.Is(err, users.ErrNotActive): return fiber.NewError(fiber.StatusForbidden, err.Error()) + case errors.Is(err, jwt.ErrInvalidToken): + return fiber.NewError(fiber.StatusUnauthorized, err.Error()) + case errors.Is(err, jwt.ErrExpiredToken): + return fiber.NewError(fiber.StatusUnauthorized, err.Error()) default: return err //nolint:wrapcheck // err is already wrapped }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/admin/users/handler.go` around lines 143 - 160, The admin errorsHandler currently maps only users.* errors; update it to also handle jwt.ErrInvalidToken and jwt.ErrExpiredToken by adding switch cases in errorsHandler that detect errors.Is(err, jwt.ErrInvalidToken) and errors.Is(err, jwt.ErrExpiredToken) and return fiber.NewError(fiber.StatusUnauthorized, err.Error())—this mirrors the auth handler behavior for routes wrapped with jwtauth.New(h.jwtSvc).
62-75: Boundlimitandoffsetbefore querying.
limitandoffsetcome straight from the request here. Negative values or an unusually largelimitcan turn a bad admin request into a repository error or a very heavy query. Reject negative offsets and caplimitbefore callingList/Count.Possible patch
func (h *Handler) handleList(c *fiber.Ctx) error { filter := defaultAdminUserFilter() if err := c.QueryParser(&filter); err != nil { return fmt.Errorf("failed to parse query: %w", err) } + + if filter.Offset < 0 { + return fiber.NewError(fiber.StatusBadRequest, "offset must be >= 0") + } + + if filter.Limit <= 0 || filter.Limit > 100 { + return fiber.NewError(fiber.StatusBadRequest, "limit must be between 1 and 100") + } // Get users from service usersList, err := h.usersSvc.List(c.Context(), filter.Status, filter.Role, filter.Limit, filter.Offset)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/admin/users/handler.go` around lines 62 - 75, After parsing into filter (defaultAdminUserFilter + c.QueryParser), validate and bound filter.Limit and filter.Offset before calling h.usersSvc.List or h.usersSvc.Count: reject negative filter.Offset with a clear error, clamp filter.Limit to a safe maximum (introduce/ use a constant like maxAdminUserLimit) and enforce a minimum of 1 if needed, then pass the sanitized values to h.usersSvc.List(here) and h.usersSvc.Count; ensure any invalid parameters return an appropriate error to the caller instead of forwarding them to the service.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/db/migrations/20260408120000_users.sql`:
- Line 5: Normalize emails to avoid case-sensitivity bugs: in Service.Register
ensure the incoming UserInput.Email is normalized (strings.ToLower) before any
repository calls or validation, and in Service.Login lowercase the email
parameter before querying by email; update all paths in the users package that
accept or compare emails to use the normalized value (refer to Service.Register,
Service.Login and repository calls that use the email). Alternatively, if you
prefer DB-level enforcement, change the `email` column to use a binary collation
(e.g., `VARCHAR(255) COLLATE utf8mb4_bin NOT NULL`) so uniqueness is
case-sensitive, but if keeping the current collation apply the lowercase
normalization consistently.
In `@internal/server/middlewares/jwtauth/jwtauth.go`:
- Around line 29-42: Don't trust mutable claims.Role/claims.Status from
jwtSvc.ValidateToken for authorization; instead use the token only to identify
the caller (claims.UserID) and then load current user state (e.g., fetch user by
ID from your user repository/service) to enforce role/status checks and admin
gating. Update the jwtauth middleware where you currently set
c.Locals(userIDKey, claims.UserID), c.Locals(roleKey, claims.Role), and
c.Locals(statusKey, claims.Status) to: store the user ID from claims, then
retrieve the authoritative user record (or validate token version/revocation)
and use that record's Role/Status for blocking (users.StatusBlocked) and
authorization decisions; ensure any other checks (the ones around lines 49-57)
are changed similarly to reference the stored user lookup or revocation/version
check rather than JWT claims.
---
Duplicate comments:
In `@internal/users/password.go`:
- Around line 70-98: The current guards before calling argon2.IDKey are
incomplete: add explicit rejects for memory == 0, iterations == 0, len(salt) ==
0 and len(hash) == 0, and ensure len(hash) fits into the uint32 cast used for
argon2.IDKey (i.e., validate that uint64(len(hash)) <= max uint32) instead of
the existing math.MaxUint16 check; update the conditional that currently
references memory, iterations, parallelism, salt and hash (variables memory,
iterations, parallelism, salt, hash) so it returns ErrInvalidCredential for any
of these malformed values before computing computedHash with argon2.IDKey.
---
Nitpick comments:
In `@internal/db/migrations/20260408120000_users.sql`:
- Line 8: The users table lacks an index on the status column, causing
ListByStatus (and any status-filter queries) to do full table scans; update the
migration in internal/db/migrations/20260408120000_users.sql to add an index on
`status` (e.g., CREATE INDEX or ALTER TABLE ... ADD INDEX `idx_users_status`
(`status`)) so ListByStatus and similar methods can use the index for efficient
filtering.
In `@internal/server/admin/users/handler.go`:
- Around line 143-160: The admin errorsHandler currently maps only users.*
errors; update it to also handle jwt.ErrInvalidToken and jwt.ErrExpiredToken by
adding switch cases in errorsHandler that detect errors.Is(err,
jwt.ErrInvalidToken) and errors.Is(err, jwt.ErrExpiredToken) and return
fiber.NewError(fiber.StatusUnauthorized, err.Error())—this mirrors the auth
handler behavior for routes wrapped with jwtauth.New(h.jwtSvc).
- Around line 62-75: After parsing into filter (defaultAdminUserFilter +
c.QueryParser), validate and bound filter.Limit and filter.Offset before calling
h.usersSvc.List or h.usersSvc.Count: reject negative filter.Offset with a clear
error, clamp filter.Limit to a safe maximum (introduce/ use a constant like
maxAdminUserLimit) and enforce a minimum of 1 if needed, then pass the sanitized
values to h.usersSvc.List(here) and h.usersSvc.Count; ensure any invalid
parameters return an appropriate error to the caller instead of forwarding them
to the service.
In `@internal/users/password.go`:
- Around line 61-68: Ensure the hash metadata tokens are validated before
parsing: after checking len(vals) != valsCount, explicitly verify that vals[1]
== "argon2id" and that vals[2] starts with "v=" (and optionally parse the
version number) and return fmt.Errorf("%w: invalid hash", ErrInvalidCredential)
if they do not match; update the logic around the vals, valsCount, and
ErrInvalidCredential checks in internal/users/password.go so malformed tokens
are rejected early (e.g., in the function handling password hash parsing).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5acf2866-f603-4f21-97b9-ca0495a54f7a
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
go.modinternal/app.gointernal/config/config.gointernal/config/module.gointernal/db/migrations/20260408120000_users.sqlinternal/jwt/config.gointernal/jwt/doc.gointernal/jwt/domain.gointernal/jwt/errors.gointernal/jwt/module.gointernal/jwt/service.gointernal/server/admin/users/dto.gointernal/server/admin/users/handler.gointernal/server/auth/dto.gointernal/server/auth/handler.gointernal/server/docs/docs.gointernal/server/middlewares/jwtauth/jwtauth.gointernal/server/module.gointernal/users/doc.gointernal/users/domain.gointernal/users/errors.gointernal/users/models.gointernal/users/module.gointernal/users/password.gointernal/users/repository.gointernal/users/service.gorequests.http
✅ Files skipped from review due to trivial changes (14)
- internal/jwt/doc.go
- internal/jwt/module.go
- internal/users/module.go
- internal/jwt/errors.go
- internal/users/errors.go
- internal/jwt/domain.go
- internal/server/auth/dto.go
- internal/users/doc.go
- requests.http
- internal/server/docs/docs.go
- internal/users/models.go
- internal/users/domain.go
- internal/server/admin/users/dto.go
- internal/users/service.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/config/module.go
- internal/server/module.go
- internal/jwt/config.go
- internal/app.go
- go.mod
- internal/jwt/service.go
- internal/config/config.go
- internal/users/repository.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/users/repository.go (1)
143-149:⚠️ Potential issue | 🟠 Major
RowsAffected()==0will misreport idempotent updates asErrNotFoundon MySQL.In this repo's MySQL setup, updating a row to the same
status/rolealso yields0affected rows. That means an existing user can be reported as missing when the request is a no-op. If you need real not-found detection here, use a preflight existence check or switch the DSN toclientFoundRows=true; otherwise treat this update as idempotent success. Based on learnings: In thebit-issues/backendrepository (MySQL),RowsAffected()returns 0 both when no row matches the WHERE clause AND when a matching row's values are unchanged by the UPDATE.In MySQL, can UPDATE report RowsAffected() = 0 when the row exists but the assigned values are unchanged, and how does clientFoundRows change that behavior?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/repository.go` around lines 143 - 149, The current check using result.RowsAffected() and returning ErrNotFound incorrectly treats idempotent MySQL updates (no changed values) as "not found"; update the code in internal/users/repository.go to avoid this: either (A) treat rows == 0 as an idempotent success (remove the ErrNotFound return and treat the update as OK), or (B) perform an explicit existence preflight (SELECT id FROM users WHERE id = ?) before the UPDATE and only return ErrNotFound if the SELECT finds no row; if you must rely on RowsAffected() for not-found detection, document and require the DSN option clientFoundRows=true so result.RowsAffected() > 0 only when a row truly did not exist. Ensure changes reference and adjust logic around result.RowsAffected() and ErrNotFound accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/auth/handler.go`:
- Around line 131-139: The switch in the error-handling block is using
err.Error() which can leak internal context; update the case arms in the switch
that checks errors.Is to return fixed public messages instead of err.Error().
Specifically, in the switch handling users.ErrEmailAlreadyUsed,
users.ErrNotFound, users.ErrInvalidCredential, and users.ErrNotActive (the
switch in handler.go), replace fiber.NewError(..., err.Error()) with
fiber.NewError(..., "email already in use"), fiber.NewError(..., "user not
found"), fiber.NewError(..., "invalid credentials"), and fiber.NewError(...,
"user not active") (or use the sentinel error's own public string if you
prefer), so the response bodies are canonical and do not expose internal error
chains.
In `@internal/server/middlewares/jwtauth/jwtauth.go`:
- Around line 28-31: The code wraps any usersSvc.GetByID error and bubbles it as
a generic error, causing deleted/missing users (users.ErrNotFound) to become
500s; change the jwtauth middleware so after calling
usersSvc.GetByID(c.Context(), claims.UserID) you check errors.Is(err,
users.ErrNotFound) and return an auth-failure (e.g., return false, nil) so it
maps to a 401, otherwise continue to wrap/return other errors as before; ensure
you import the errors package and reference users.ErrNotFound, usersSvc.GetByID,
and claims.UserID in the fix.
---
Duplicate comments:
In `@internal/users/repository.go`:
- Around line 143-149: The current check using result.RowsAffected() and
returning ErrNotFound incorrectly treats idempotent MySQL updates (no changed
values) as "not found"; update the code in internal/users/repository.go to avoid
this: either (A) treat rows == 0 as an idempotent success (remove the
ErrNotFound return and treat the update as OK), or (B) perform an explicit
existence preflight (SELECT id FROM users WHERE id = ?) before the UPDATE and
only return ErrNotFound if the SELECT finds no row; if you must rely on
RowsAffected() for not-found detection, document and require the DSN option
clientFoundRows=true so result.RowsAffected() > 0 only when a row truly did not
exist. Ensure changes reference and adjust logic around result.RowsAffected()
and ErrNotFound accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fd998a5-8c3b-4bcb-9441-a076af112a82
📒 Files selected for processing (10)
internal/server/admin/users/dto.gointernal/server/admin/users/handler.gointernal/server/auth/handler.gointernal/server/middlewares/jwtauth/jwtauth.gointernal/server/module.gointernal/users/domain.gointernal/users/models.gointernal/users/password.gointernal/users/repository.gointernal/users/service.go
✅ Files skipped from review due to trivial changes (1)
- internal/server/admin/users/handler.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/server/module.go
- internal/users/password.go
- internal/users/models.go
- internal/server/admin/users/dto.go
capcom6
force-pushed
the
codex/design-users-module
branch
from
d9e93a3 to
65ed904
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/users/password.go (1)
90-101:⚠️ Potential issue | 🔴 CriticalReject zero-length decoded credentials before hashing/comparing.
This guard is still incomplete.
memory == 0,iterations == 0,len(salt) == 0, and especiallylen(hash) == 0all slip through toargon2.IDKey. Malformed stored hashes should be rejected up front asErrInvalidCredentialinstead of entering the recompute/compare path.Suggested fix
- if parallelism == 0 || parallelism > math.MaxUint8 || len(hash) > math.MaxUint16 { + if memory == 0 || iterations == 0 || parallelism == 0 || parallelism > math.MaxUint8 || + len(salt) == 0 || len(hash) == 0 || len(hash) > math.MaxUint32 { return fmt.Errorf("%w: invalid hash", ErrInvalidCredential) }In golang.org/x/crypto/argon2 v0.49.0, what does argon2.IDKey return when keyLen is 0, and does crypto/subtle.ConstantTimeCompare return 1 when comparing two empty byte slices?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/users/password.go` around lines 90 - 101, The stored-credential validation in internal/users/password.go is incomplete: before calling argon2.IDKey and crypto/subtle.ConstantTimeCompare, reject malformed parameters by checking that iterations>0, memory>0, len(salt)>0 and len(hash)>0 (in addition to the existing parallelism and size checks) and return ErrInvalidCredential on any invalid value; update the same guard that currently checks parallelism/len(hash) (used around computedHash := argon2.IDKey(...)) to include these non-zero checks so you never call argon2.IDKey or ConstantTimeCompare with zero-length inputs.internal/server/auth/handler.go (1)
131-139:⚠️ Potential issue | 🟡 MinorReturn canonical auth messages here, not wrapped error chains.
These branches classify the errors correctly, but
err.Error()leaks handler/service prefixes into the response body. Return the sentinel text instead.Suggested fix
switch { case errors.Is(err, users.ErrEmailAlreadyUsed): - return fiber.NewError(fiber.StatusConflict, err.Error()) + return fiber.NewError(fiber.StatusConflict, users.ErrEmailAlreadyUsed.Error()) case errors.Is(err, users.ErrNotFound): - return fiber.NewError(fiber.StatusNotFound, err.Error()) + return fiber.NewError(fiber.StatusNotFound, users.ErrNotFound.Error()) case errors.Is(err, users.ErrInvalidCredential): - return fiber.NewError(fiber.StatusUnauthorized, err.Error()) + return fiber.NewError(fiber.StatusUnauthorized, users.ErrInvalidCredential.Error()) case errors.Is(err, users.ErrNotActive): - return fiber.NewError(fiber.StatusForbidden, err.Error()) + return fiber.NewError(fiber.StatusForbidden, users.ErrNotActive.Error())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth/handler.go` around lines 131 - 139, The handler is returning err.Error() which leaks service/handler prefixes; instead return the canonical sentinel messages. For each branch that does errors.Is(err, users.ErrEmailAlreadyUsed / users.ErrNotFound / users.ErrInvalidCredential / users.ErrNotActive) replace the second argument to fiber.NewError with the sentinel's own message (e.g. users.ErrEmailAlreadyUsed.Error(), users.ErrNotFound.Error(), users.ErrInvalidCredential.Error(), users.ErrNotActive.Error()) so the response body contains only the canonical text rather than the wrapped error chain.internal/server/middlewares/jwtauth/jwtauth.go (1)
28-31:⚠️ Potential issue | 🟠 MajorDon't collapse every user lookup failure into
401 user not found.Only
users.ErrNotFoundshould become an auth failure here. A DB error or timeout currently gets misclassified as unauthorized, which hides operational failures and makes debugging much harder.Suggested fix
user, err := usersSvc.GetByID(c.Context(), claims.UserID) if err != nil { - return false, fiber.NewError(fiber.StatusUnauthorized, "user not found") + if errors.Is(err, users.ErrNotFound) { + return false, fiber.NewError(fiber.StatusUnauthorized, "user not found") + } + + return false, fmt.Errorf("failed to load user: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/middlewares/jwtauth/jwtauth.go` around lines 28 - 31, The current lookup in usersSvc.GetByID collapses all errors to a 401; change the error handling in the middleware (where usersSvc.GetByID(c.Context(), claims.UserID) is called) to only map users.ErrNotFound to fiber.NewError(fiber.StatusUnauthorized, "user not found") and for any other error return/pass the original error (or wrap as a 500/internal server error) so DB/timeouts are not misclassified; use an errors.Is(err, users.ErrNotFound) check to detect not-found vs other failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/db/migrations/20260408120000_users.sql`:
- Line 15: Remove the stray '---' line from the migration Up section in the
20260408120000_users SQL migration; this token is not a valid MySQL comment and
Goose will try to parse it as SQL. Edit the migration file and delete the
standalone '---' line so only valid SQL and proper comments (e.g., -- with a
trailing space) remain, then re-run the migration to confirm the syntax error is
resolved.
In `@internal/server/admin/users/handler.go`:
- Around line 154-162: The error mapper currently returns err.Error(), leaking
internal prefixes; update the switch that checks users.ErrEmailAlreadyUsed,
users.ErrNotFound, users.ErrInvalidCredential, and users.ErrNotActive to return
stable public messages via fiber.NewError (e.g., "email already in use", "user
not found", "invalid credentials", "user not active") instead of err.Error();
keep the same HTTP status codes and existing error sentinel checks so that the
public message is used once errors.Is(...) matches each sentinel.
---
Duplicate comments:
In `@internal/server/auth/handler.go`:
- Around line 131-139: The handler is returning err.Error() which leaks
service/handler prefixes; instead return the canonical sentinel messages. For
each branch that does errors.Is(err, users.ErrEmailAlreadyUsed /
users.ErrNotFound / users.ErrInvalidCredential / users.ErrNotActive) replace the
second argument to fiber.NewError with the sentinel's own message (e.g.
users.ErrEmailAlreadyUsed.Error(), users.ErrNotFound.Error(),
users.ErrInvalidCredential.Error(), users.ErrNotActive.Error()) so the response
body contains only the canonical text rather than the wrapped error chain.
In `@internal/server/middlewares/jwtauth/jwtauth.go`:
- Around line 28-31: The current lookup in usersSvc.GetByID collapses all errors
to a 401; change the error handling in the middleware (where
usersSvc.GetByID(c.Context(), claims.UserID) is called) to only map
users.ErrNotFound to fiber.NewError(fiber.StatusUnauthorized, "user not found")
and for any other error return/pass the original error (or wrap as a
500/internal server error) so DB/timeouts are not misclassified; use an
errors.Is(err, users.ErrNotFound) check to detect not-found vs other failures.
In `@internal/users/password.go`:
- Around line 90-101: The stored-credential validation in
internal/users/password.go is incomplete: before calling argon2.IDKey and
crypto/subtle.ConstantTimeCompare, reject malformed parameters by checking that
iterations>0, memory>0, len(salt)>0 and len(hash)>0 (in addition to the existing
parallelism and size checks) and return ErrInvalidCredential on any invalid
value; update the same guard that currently checks parallelism/len(hash) (used
around computedHash := argon2.IDKey(...)) to include these non-zero checks so
you never call argon2.IDKey or ConstantTimeCompare with zero-length inputs.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43fdfd53-c4bd-4853-be07-fe907e90fbc2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
go.modinternal/app.gointernal/config/config.gointernal/config/module.gointernal/db/migrations/20260408120000_users.sqlinternal/jwt/config.gointernal/jwt/doc.gointernal/jwt/domain.gointernal/jwt/errors.gointernal/jwt/module.gointernal/jwt/service.gointernal/server/admin/users/dto.gointernal/server/admin/users/handler.gointernal/server/auth/dto.gointernal/server/auth/handler.gointernal/server/docs/docs.gointernal/server/middlewares/jwtauth/jwtauth.gointernal/server/module.gointernal/users/doc.gointernal/users/domain.gointernal/users/errors.gointernal/users/models.gointernal/users/module.gointernal/users/password.gointernal/users/repository.gointernal/users/service.gorequests.http
✅ Files skipped from review due to trivial changes (11)
- internal/users/doc.go
- internal/users/errors.go
- internal/jwt/errors.go
- internal/server/auth/dto.go
- requests.http
- internal/server/docs/docs.go
- internal/users/models.go
- internal/jwt/doc.go
- internal/server/admin/users/dto.go
- internal/users/domain.go
- internal/jwt/domain.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/app.go
- internal/jwt/module.go
- internal/config/module.go
- internal/server/module.go
- internal/jwt/config.go
- go.mod
- internal/config/config.go
- internal/users/module.go
capcom6
force-pushed
the
codex/design-users-module
branch
from
cfbfd12 to
1d83a20
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
internal/server/admin/users/dto.go (1)
61-69:⚠️ Potential issue | 🟡 MinorGuard mapper against nil input to prevent panic paths.
Line 62 dereferences
uunconditionally. If a nil user ever slips through, this will panic the handler path.Suggested fix
func toUserResponse(u *users.User) UserResponse { + if u == nil { + return UserResponse{} + } return UserResponse{ ID: u.ID, Email: u.Email, Role: u.Role, Status: u.Status, CreatedAt: u.CreatedAt, UpdatedAt: u.UpdatedAt, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/admin/users/dto.go` around lines 61 - 69, The toUserResponse function dereferences its parameter unconditionally and can panic if passed a nil *users.User; add a nil guard at the top of toUserResponse (check if u == nil) and return an empty UserResponse (zero-value) immediately when nil to prevent panics, keeping the rest of the mapping for the non-nil case that copies fields from *users.User to UserResponse.internal/server/auth/handler.go (1)
131-139:⚠️ Potential issue | 🟡 MinorReturn canonical public auth messages instead of full wrapped errors.
Lines 133/135/137/139 use
err.Error(), which can leak internal wrapper context (e.g.,failed to login user: ...) in client responses.Suggested fix
switch { case errors.Is(err, users.ErrEmailAlreadyUsed): - return fiber.NewError(fiber.StatusConflict, err.Error()) + return fiber.NewError(fiber.StatusConflict, users.ErrEmailAlreadyUsed.Error()) case errors.Is(err, users.ErrNotFound): - return fiber.NewError(fiber.StatusNotFound, err.Error()) + return fiber.NewError(fiber.StatusNotFound, users.ErrNotFound.Error()) case errors.Is(err, users.ErrInvalidCredential): - return fiber.NewError(fiber.StatusUnauthorized, err.Error()) + return fiber.NewError(fiber.StatusUnauthorized, users.ErrInvalidCredential.Error()) case errors.Is(err, users.ErrNotActive): - return fiber.NewError(fiber.StatusForbidden, err.Error()) + return fiber.NewError(fiber.StatusForbidden, users.ErrNotActive.Error())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/auth/handler.go` around lines 131 - 139, The switch in auth handler currently returns wrapped error strings (err.Error()) which may leak internal context; update the cases in the switch that check errors.Is(err, users.ErrEmailAlreadyUsed), users.ErrNotFound, users.ErrInvalidCredential, and users.ErrNotActive to return canonical public messages (e.g., "email already in use", "user not found", "invalid credentials", "account not active") instead of err.Error() when creating the fiber.NewError responses so clients only see safe, stable auth messages.internal/server/admin/users/handler.go (1)
132-134:⚠️ Potential issue | 🟡 MinorAvoid leaking wrapped internal errors in admin responses.
Line 133 and Lines 156/158/160/162 send
err.Error()/updErr.Error()to clients, exposing internal prefixes instead of stable public messages.Suggested fix
if errors.Is(updErr, users.ErrNotFound) { - return fiber.NewError(fiber.StatusNotFound, updErr.Error()) + return fiber.NewError(fiber.StatusNotFound, users.ErrNotFound.Error()) } @@ switch { case errors.Is(err, users.ErrEmailAlreadyUsed): - return fiber.NewError(fiber.StatusConflict, err.Error()) + return fiber.NewError(fiber.StatusConflict, users.ErrEmailAlreadyUsed.Error()) case errors.Is(err, users.ErrNotFound): - return fiber.NewError(fiber.StatusNotFound, err.Error()) + return fiber.NewError(fiber.StatusNotFound, users.ErrNotFound.Error()) case errors.Is(err, users.ErrInvalidCredential): - return fiber.NewError(fiber.StatusUnauthorized, err.Error()) + return fiber.NewError(fiber.StatusUnauthorized, users.ErrInvalidCredential.Error()) case errors.Is(err, users.ErrNotActive): - return fiber.NewError(fiber.StatusForbidden, err.Error()) + return fiber.NewError(fiber.StatusForbidden, users.ErrNotActive.Error())Also applies to: 154-162
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/admin/users/handler.go` around lines 132 - 134, The handler currently returns wrapped internal error strings (using updErr.Error() and err.Error()) directly to clients; change those fiber.NewError responses to return stable public messages (e.g., when errors.Is(updErr, users.ErrNotFound) return fiber.NewError(fiber.StatusNotFound, "user not found") and for other failures return fiber.NewError(fiber.StatusInternalServerError, "internal server error")), and instead log the original error (updErr/err) server-side for debugging; update all places where fiber.NewError is called with err.Error()/updErr.Error() to use fixed, public messages and retain internal logging.internal/server/middlewares/jwtauth/jwtauth.go (1)
28-31:⚠️ Potential issue | 🟠 MajorDo not convert all user lookup errors into 401.
Lines 29-31 treat every
GetByIDfailure as “user not found”. This hides backend faults (DB/network/timeout) as auth failures and makes incident diagnosis harder.Suggested fix
user, err := usersSvc.GetByID(c.Context(), claims.UserID) if err != nil { - return false, fiber.NewError(fiber.StatusUnauthorized, "user not found") + if errors.Is(err, users.ErrNotFound) { + return false, fiber.NewError(fiber.StatusUnauthorized, "user not found") + } + return false, fmt.Errorf("failed to get user: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/middlewares/jwtauth/jwtauth.go` around lines 28 - 31, The current jwtauth middleware converts every usersSvc.GetByID(c.Context(), claims.UserID) error into a 401 "user not found"; change it to detect a not-found case (e.g., compare err to usersSvc.ErrNotFound or whatever sentinel/IsNotFound helper the users service exposes) and only return fiber.StatusUnauthorized for that specific case, while returning or wrapping other errors as an internal server error (fiber.StatusInternalServerError) so DB/network/timeouts aren’t masked; update the logic in the jwtauth handler around the GetByID call (use claims.UserID for context) to branch on the error type and return the appropriate Fiber error.internal/config/config.go (1)
65-67:⚠️ Potential issue | 🔴 CriticalDefault JWT secret is insecure; enforce minimum length at config load.
Line 66 uses a hardcoded weak default (
"secret"). This materially weakens token signing and should fail fast if not explicitly configured.Suggested fix
func Default() Config { //nolint:gosec,mnd // default values return Config{ @@ JWT: jwtConfig{ - Secret: "secret", + Secret: "", AccessTTL: time.Minute * 15, Issuer: "bitissues.dev", }, } } @@ func New() (Config, error) { @@ if err := config.Load(&cfg, options...); err != nil { return Config{}, fmt.Errorf("failed to load config: %w", err) } + if len(cfg.JWT.Secret) < 32 { + return Config{}, fmt.Errorf("failed to load config: jwt.secret must be at least 32 bytes") + } return cfg, nil }Also applies to: 73-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 65 - 67, The default JWT secret is insecure (hardcoded "secret") in the jwtConfig values; remove the weak literal and enforce a minimum length check during config load (e.g., in your LoadConfig/NewConfig function) for JWT.Secret (the JWT and jwtConfig symbols) — validate that Secret is non-empty and at least a safe length (e.g., 32 bytes) and return an error (or fail fast) if it is missing or too short; update any other default occurrences (the other JWT defaults referenced around lines 73-85) to not supply a weak secret.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/server/auth/dto.go`:
- Around line 54-62: toUserResponseDTO currently dereferences the input
users.User without a nil check which can panic; add a nil guard at the start of
toUserResponseDTO (check if u == nil) and return an empty UserResponseDTO{} when
nil to avoid panics, keeping the function signature unchanged; reference
users.User and UserResponseDTO inside the function to locate where to add the
guard.
---
Duplicate comments:
In `@internal/config/config.go`:
- Around line 65-67: The default JWT secret is insecure (hardcoded "secret") in
the jwtConfig values; remove the weak literal and enforce a minimum length check
during config load (e.g., in your LoadConfig/NewConfig function) for JWT.Secret
(the JWT and jwtConfig symbols) — validate that Secret is non-empty and at least
a safe length (e.g., 32 bytes) and return an error (or fail fast) if it is
missing or too short; update any other default occurrences (the other JWT
defaults referenced around lines 73-85) to not supply a weak secret.
In `@internal/server/admin/users/dto.go`:
- Around line 61-69: The toUserResponse function dereferences its parameter
unconditionally and can panic if passed a nil *users.User; add a nil guard at
the top of toUserResponse (check if u == nil) and return an empty UserResponse
(zero-value) immediately when nil to prevent panics, keeping the rest of the
mapping for the non-nil case that copies fields from *users.User to
UserResponse.
In `@internal/server/admin/users/handler.go`:
- Around line 132-134: The handler currently returns wrapped internal error
strings (using updErr.Error() and err.Error()) directly to clients; change those
fiber.NewError responses to return stable public messages (e.g., when
errors.Is(updErr, users.ErrNotFound) return fiber.NewError(fiber.StatusNotFound,
"user not found") and for other failures return
fiber.NewError(fiber.StatusInternalServerError, "internal server error")), and
instead log the original error (updErr/err) server-side for debugging; update
all places where fiber.NewError is called with err.Error()/updErr.Error() to use
fixed, public messages and retain internal logging.
In `@internal/server/auth/handler.go`:
- Around line 131-139: The switch in auth handler currently returns wrapped
error strings (err.Error()) which may leak internal context; update the cases in
the switch that check errors.Is(err, users.ErrEmailAlreadyUsed),
users.ErrNotFound, users.ErrInvalidCredential, and users.ErrNotActive to return
canonical public messages (e.g., "email already in use", "user not found",
"invalid credentials", "account not active") instead of err.Error() when
creating the fiber.NewError responses so clients only see safe, stable auth
messages.
In `@internal/server/middlewares/jwtauth/jwtauth.go`:
- Around line 28-31: The current jwtauth middleware converts every
usersSvc.GetByID(c.Context(), claims.UserID) error into a 401 "user not found";
change it to detect a not-found case (e.g., compare err to usersSvc.ErrNotFound
or whatever sentinel/IsNotFound helper the users service exposes) and only
return fiber.StatusUnauthorized for that specific case, while returning or
wrapping other errors as an internal server error
(fiber.StatusInternalServerError) so DB/network/timeouts aren’t masked; update
the logic in the jwtauth handler around the GetByID call (use claims.UserID for
context) to branch on the error type and return the appropriate Fiber error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14c4f9cd-83d9-4317-9ebc-4f58de2f8282
📒 Files selected for processing (11)
internal/app.gointernal/config/config.gointernal/config/module.gointernal/server/admin/users/dto.gointernal/server/admin/users/handler.gointernal/server/auth/dto.gointernal/server/auth/handler.gointernal/server/docs/docs.gointernal/server/middlewares/jwtauth/jwtauth.gointernal/server/module.gorequests.http
✅ Files skipped from review due to trivial changes (2)
- requests.http
- internal/server/docs/docs.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/app.go
- internal/server/module.go
capcom6
changed the title
This was referenced Apr 13, 2026
Merged
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.
Motivation
Description
usersmodule and register it ininternal/app.goviausers.Module()to wire service, repository, and handlers into the app.storage_model.goanddomain.go, DTOs and response conversion indto.go, and error definitions inerrors.go.repository.gowith CRUD-like operations (Create,GetByEmail,GetByID,ListByStatus,UpdateStatus,UpdatePasswordHash) and duplicate-key detection; implement business logic inservice.gousingbcryptfor password hashing/verification.handler.goexposing routes under/usersforPOST /register,POST /login,GET /users,PATCH /users/:id/status, andPOST /users/:id/change-passwordwith validation and appropriate error mapping.internal/db/migrations/20260408120000_users.sqlto create theuserstable with email uniqueness, enums forroleandstatus, and timestamp columns.Testing
Codex Task
Summary by CodeRabbit
New Features
Database
Configuration
Documentation
Security