Conversation
WalkthroughAdds authentication and user management: new DB and JWT config entries; JWT utilities for token generation and validation; an auth service (register/login with password hashing and token issuance); a users service (paginated list and delete); SQLC queries for create/delete/paginated list/count; HTTP DTOs, handlers, and middleware (JWT + role checks); router wiring protecting /users endpoints; system integration tests using a Postgres testcontainer; and dependency updates in go.mod. Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthHandler
participant AuthService
participant Database
participant JWT
Client->>AuthHandler: POST /auth/register (username,password)
AuthHandler->>AuthService: Register(username,password)
AuthService->>AuthService: Hash password
AuthService->>Database: CreateUser(username,password_hash,role)
Database-->>AuthService: User row (id,username,role,created_at)
AuthService-->>AuthHandler: RegisterResult (id,username,role)
AuthHandler-->>Client: 201 RegisterResponse
sequenceDiagram
participant Client
participant AuthHandler
participant AuthService
participant Database
participant JWT
Client->>AuthHandler: POST /auth/login (username,password)
AuthHandler->>AuthService: Login(username,password)
AuthService->>Database: GetUserByUsername(username)
Database-->>AuthService: User (id,username,role,password_hash)
AuthService->>AuthService: Verify password
alt password valid
AuthService->>JWT: GenerateToken(cfg,userID,username,role)
JWT-->>AuthService: token
AuthService-->>AuthHandler: token
AuthHandler-->>Client: 200 LoginResponse (token)
else invalid
AuthService-->>AuthHandler: ErrInvalidCredentials
AuthHandler-->>Client: 401 Unauthorized
end
sequenceDiagram
participant Client
participant JWTMiddleware
participant UserHandler
participant UserService
participant Database
Client->>JWTMiddleware: GET /users (Authorization: Bearer <token>)
JWTMiddleware->>JWTMiddleware: Validate token, set ctx user_id & role
JWTMiddleware->>UserHandler: pass request
UserHandler->>UserService: ListUsers(limit,offset)
UserService->>Database: ListUsersPaginated(limit,offset)
Database-->>UserService: []User rows
UserService->>Database: CountUsers()
Database-->>UserService: totalCount
UserService-->>UserHandler: users,total
UserHandler-->>Client: 200 ListUsersResponse
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/silo-proxy-server/config.go (1)
70-75:⚠️ Potential issue | 🟠 MajorSensitive credentials (DB URL with password, JWT secret) will be logged at DEBUG level.
The
json.MarshalIndent(config, ...)call serializes the entireConfigstruct, which now includesDB.URL(containing the Postgres password) andJWT.Secret. Even at DEBUG level, this leaks secrets into stdout/log sinks.Consider redacting sensitive fields before logging, or implementing a custom
MarshalJSONonConfig/ the nested types that masks secrets.
🤖 Fix all issues with AI agents
In `@cmd/silo-proxy-server/application.yml`:
- Around line 3-8: Replace the hardcoded DB URL and JWT secret in
application.yml with non-functional placeholders or env-var references and
document the required env vars (DB_URL and JWT_SECRET) so secrets are not
committed; update the config usage in InitConfig (which calls
viper.AutomaticEnv()) to read DB_URL and JWT_SECRET environment variables (and
fallback to safe placeholders only for local dev), and add a .env.example or
README section listing DB_URL, DB_SCHEMA, JWT_SECRET, and JWT_EXPIRATION_MINUTES
with guidance for production rotation to satisfy the static scanner (Checkov
CKV_SECRET_4) and prevent plaintext credentials in VCS.
In `@go.mod`:
- Line 83: The go.mod currently lists github.com/quic-go/quic-go v0.54.0
(indirect) which has high-severity vulnerabilities; update the dependency by
either (A) explicitly requiring github.com/quic-go/quic-go at v0.57.0 or later
(e.g., v0.59.0) in go.mod, or (B) bump github.com/gin-gonic/gin to a release
that transitively pins quic-go >= v0.57.0; after changing go.mod run module
housekeeping (go mod tidy) and rebuild/run tests to ensure the new quic-go
version is used and no compatibility issues surface.
- Line 8: Update the golang.org/x/crypto dependency from v0.46.0 to v0.47.0 in
the go.mod entry for "golang.org/x/crypto": change the version string to
v0.47.0, then run the module tooling (e.g., go get golang.org/x/crypto@v0.47.0
and go mod tidy) to ensure go.sum is updated and imports resolve correctly;
verify no other files reference the old version.
In `@internal/api/http/router.go`:
- Around line 24-29: The code constructs an auth handler unconditionally which
can panic if srvs.AuthService is nil; add a nil guard similar to the GrpcServer
check: only call handler.NewAuthHandler(srvs.AuthService) and register
authRoutes when srvs.AuthService != nil, otherwise skip route registration (or
log/return); adjust references in router.go around AuthService, NewAuthHandler,
authRoutes and mirror the nil-check pattern used for GrpcServer to avoid
dereferencing a nil service at runtime.
In `@internal/auth/service.go`:
- Around line 63-74: The Login function leaks username existence via timing:
when s.queries.GetUserByUsername returns pgx.ErrNoRows you return immediately
but when a user exists you call users.CheckPassword which invokes bcrypt and
takes much longer; to fix, in the GetUserByUsername error path perform a dummy
bcrypt comparison using a constant precomputed hash (e.g., call
bcrypt.CompareHashAndPassword on a package-level fakeHash) before returning
ErrInvalidCredentials so both branches execute a slow bcrypt operation, keeping
symbols to modify: Service.Login, s.queries.GetUserByUsername,
users.CheckPassword/ErrInvalidCredentials and introduce a package-level fakeHash
used for the dummy compare.
In `@systemtest/main_test.go`:
- Around line 26-44: Replace setup assertions with fail-fast checks: change
assert.NoError to require.NoError for StartPostgres, container.MappedPort,
db.RunMigrations and db.InitDB so the test aborts immediately on setup failures
(referencing StartPostgres, container.MappedPort, db.RunMigrations, db.InitDB
and the resulting variables container and pool). Keep the TerminatePostgres call
in the deferred cleanup but retain a non-fatal check there (leave the existing
assert.NoError inside the defer) and ensure pool.Close remains deferred after
require.InitDB succeeds so you don't call Close on a nil pool.
🧹 Nitpick comments (9)
systemtest/postgres/postgres.go (1)
19-23: 5-second startup timeout may be too aggressive for CI.The wait strategy requires 2 occurrences of the "ready" log line (correct for Postgres), but 5 seconds can be tight in CI environments with cold Docker caches or resource contention. Consider bumping to 30–60 seconds.
Proposed fix
testcontainers.WithWaitStrategy( wait.ForLog("database system is ready to accept connections"). WithOccurrence(2). - WithStartupTimeout(5*time.Second)), + WithStartupTimeout(60*time.Second)),systemtest/main_test.go (1)
57-59: Test ordering dependency betweenRegisterandLoginis implicit.
TestLogindepends on a shared database with users created byTestRegister(and its own setup registration at Line 55-57 ofauth.go). Currently this works becauset.Runexecutes sequentially, but adding a comment or explicitly seeding in each test would make the dependency clearer and more robust.internal/auth/jwt.go (1)
17-24: Consider validatingConfigfields at construction time.If
Secretis empty orExpirationMinutesis ≤ 0, tokens will be signed with an empty key or expire immediately. A constructor or validation method onConfig(or inNewService) would catch misconfiguration early at startup rather than producing silently broken tokens at runtime.Example validation
func (c Config) Validate() error { if len(c.Secret) < 32 { return fmt.Errorf("jwt secret must be at least 32 characters") } if c.ExpirationMinutes <= 0 { return fmt.Errorf("jwt expiration must be positive") } return nil }internal/api/http/handler/auth.go (1)
21-65: Consider rate-limiting the auth endpoints.
/auth/registerand/auth/loginare unauthenticated and involve bcrypt hashing, making them targets for brute-force and credential-stuffing attacks. Adding rate limiting (e.g., per-IP or per-username) would mitigate this.cmd/silo-proxy-server/main.go (1)
42-42:defer dbPool.Close()will be skipped if a lateros.Exit(1)fires.Lines 69 and 82 call
os.Exit(1)on cert/port-manager failures, which bypass all deferred calls includingdbPool.Close(). This is a pre-existing pattern in the file, but worth noting — consider restructuring startup errors to return frommain()(via arun()function) instead of callingos.Exitdirectly, so deferred cleanup always executes.systemtest/tests/auth.go (1)
88-95: Silently ignoredjson.Marshalerror.Line 89 discards the marshal error. While unlikely to fail for these DTOs, propagating the error via
requirewould catch unexpected issues (e.g., if a test accidentally passes an unmarshallable value).Proposed fix
The function would need
t *testing.Tas a parameter:-func doJSON(router *gin.Engine, method, path string, body any) *httptest.ResponseRecorder { - b, _ := json.Marshal(body) +func doJSON(t *testing.T, router *gin.Engine, method, path string, body any) *httptest.ResponseRecorder { + t.Helper() + b, err := json.Marshal(body) + require.NoError(t, err)Then update all call sites to pass
t.internal/api/http/dto/auth.go (1)
14-17: Consider adding amaxconstraint onLoginRequest.Password.bcrypt silently truncates input beyond 72 bytes, and processing very large payloads through bcrypt is CPU-intensive. Adding a reasonable
max(e.g.,max=128) guards against accidental or malicious oversized inputs.Proposed fix
type LoginRequest struct { Username string `json:"username" binding:"required"` - Password string `json:"password" binding:"required"` + Password string `json:"password" binding:"required,max=128"` }Apply the same
maxtoRegisterRequest.Passwordfor consistency:- Password string `json:"password" binding:"required,min=8"` + Password string `json:"password" binding:"required,min=8,max=128"`internal/auth/service.go (2)
37-54: Consider adding input validation for username and password.
Registerdoes not validate thatusernameandpasswordare non-empty (or meet minimum length/complexity requirements) before hashing and hitting the database. An empty password would silently produce a valid bcrypt hash, and an empty username would depend entirely on DB constraints.🛡️ Proposed validation
func (s *Service) Register(ctx context.Context, username, password string) (RegisterResult, error) { + if username == "" { + return RegisterResult{}, fmt.Errorf("username must not be empty") + } + if len(password) < 8 { + return RegisterResult{}, fmt.Errorf("password must be at least 8 characters") + } + hash, err := users.HashPassword(password)
84-87: Prefer a UUID library over manual formatting.Rolling your own UUID-to-string conversion is fragile.
google/uuidis already a project dependency and used elsewhere in the codebase. Replace the manualfmt.Sprintfwithuuid.UUID(id).String().♻️ Suggested simplification
+import "github.com/google/uuid" + func uuidToString(id [16]byte) string { - return fmt.Sprintf("%08x-%04x-%04x-%04x-%012x", - id[0:4], id[4:6], id[6:8], id[8:10], id[10:16]) + return uuid.UUID(id).String() }
| db: | ||
| url: "postgres://postgres:postgres@localhost:5432/silo?sslmode=disable" | ||
| schema: "public" | ||
| jwt: | ||
| secret: "change-me-to-a-random-secret" | ||
| expiration_minutes: 1440 |
There was a problem hiding this comment.
Hardcoded credentials and JWT secret placeholder committed to VCS.
The DB URL on Line 4 contains plaintext credentials (postgres:postgres) and the JWT secret on Line 7 is a well-known placeholder. While viper.AutomaticEnv() in InitConfig allows env-var overrides (e.g., DB_URL, JWT_SECRET), consider:
- Replacing the DB URL with an env-var reference or a clearly non-functional placeholder to avoid accidental use in non-dev environments.
- Documenting the required env-var overrides for production (e.g., in a README or
.env.example).
The static analysis tool (Checkov CKV_SECRET_4) also flagged the credentials on lines 4-5.
🧰 Tools
🪛 Checkov (3.2.334)
[medium] 4-5: Basic Auth Credentials
(CKV_SECRET_4)
🤖 Prompt for AI Agents
In `@cmd/silo-proxy-server/application.yml` around lines 3 - 8, Replace the
hardcoded DB URL and JWT secret in application.yml with non-functional
placeholders or env-var references and document the required env vars (DB_URL
and JWT_SECRET) so secrets are not committed; update the config usage in
InitConfig (which calls viper.AutomaticEnv()) to read DB_URL and JWT_SECRET
environment variables (and fallback to safe placeholders only for local dev),
and add a .env.example or README section listing DB_URL, DB_SCHEMA, JWT_SECRET,
and JWT_EXPIRATION_MINUTES with guidance for production rotation to satisfy the
static scanner (Checkov CKV_SECRET_4) and prevent plaintext credentials in VCS.
| require ( | ||
| github.com/gin-contrib/cors v1.7.6 | ||
| github.com/gin-gonic/gin v1.11.0 | ||
| github.com/golang-jwt/jwt/v5 v5.3.1 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
latest version golang-jwt/jwt/v5 2026
💡 Result:
As of February 6, 2026, the latest release of github.com/golang-jwt/jwt/v5 is v5.3.1 (released Jan 28). [1]
Upgrade/install: go get github.com/golang-jwt/jwt/v5@v5.3.1 [1]
Sources:
[1] GitHub Releases: golang-jwt/jwt – Release v5.3.1
🌐 Web query:
latest version golang.org/x/crypto 2026
💡 Result:
As of February 6, 2026, the latest tagged release of golang.org/x/crypto is v0.47.0 (published Jan 12, 2026). [1]
🌐 Web query:
latest version google.golang.org/protobuf 2026
💡 Result:
Latest tagged release of google.golang.org/protobuf (as of Feb 6, 2026) is v1.36.11 (released Dec 12, 2025). [1][2]
To update:
go get google.golang.org/protobuf@v1.36.11Sources: [1] (github.com) [2] (pkg.go.dev)
Citations:
- 1: https://github.com/protocolbuffers/protobuf-go/releases
- 2: https://pkg.go.dev/google.golang.org/protobuf/internal/version?utm_source=openai
Update golang.org/x/crypto to v0.47.0 (currently v0.46.0). The other two dependencies—github.com/golang-jwt/jwt/v5 (v5.3.1) and google.golang.org/protobuf (v1.36.11)—are already at their latest versions.
Also applies to: 8-8, 19-19
🤖 Prompt for AI Agents
In `@go.mod` at line 8, Update the golang.org/x/crypto dependency from v0.46.0 to
v0.47.0 in the go.mod entry for "golang.org/x/crypto": change the version string
to v0.47.0, then run the module tooling (e.g., go get
golang.org/x/crypto@v0.47.0 and go mod tidy) to ensure go.sum is updated and
imports resolve correctly; verify no other files reference the old version.
| func (s *Service) Login(ctx context.Context, username, password string) (string, error) { | ||
| user, err := s.queries.GetUserByUsername(ctx, username) | ||
| if err != nil { | ||
| if errors.Is(err, pgx.ErrNoRows) { | ||
| return "", ErrInvalidCredentials | ||
| } | ||
| return "", fmt.Errorf("query user: %w", err) | ||
| } | ||
|
|
||
| if !users.CheckPassword(password, user.PasswordHash) { | ||
| return "", ErrInvalidCredentials | ||
| } |
There was a problem hiding this comment.
Timing side-channel allows username enumeration.
When the user is not found (Line 67), the function returns immediately. When the user exists but the password is wrong (Line 72-73), a bcrypt comparison runs first. The ~100ms+ difference between these two paths lets an attacker determine whether a username is registered.
Mitigate by performing a dummy bcrypt.CompareHashAndPassword against a pre-computed hash on the not-found path, so both branches take comparable time.
🛡️ Proposed fix
+// dummyHash is a pre-computed bcrypt hash used to prevent timing-based username enumeration.
+var dummyHash, _ = users.HashPassword("dummy-password-for-timing")
+
func (s *Service) Login(ctx context.Context, username, password string) (string, error) {
user, err := s.queries.GetUserByUsername(ctx, username)
if err != nil {
if errors.Is(err, pgx.ErrNoRows) {
+ // Perform a dummy check to equalize timing with the real-password path
+ users.CheckPassword(password, dummyHash)
return "", ErrInvalidCredentials
}
return "", fmt.Errorf("query user: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *Service) Login(ctx context.Context, username, password string) (string, error) { | |
| user, err := s.queries.GetUserByUsername(ctx, username) | |
| if err != nil { | |
| if errors.Is(err, pgx.ErrNoRows) { | |
| return "", ErrInvalidCredentials | |
| } | |
| return "", fmt.Errorf("query user: %w", err) | |
| } | |
| if !users.CheckPassword(password, user.PasswordHash) { | |
| return "", ErrInvalidCredentials | |
| } | |
| // dummyHash is a pre-computed bcrypt hash used to prevent timing-based username enumeration. | |
| var dummyHash, _ = users.HashPassword("dummy-password-for-timing") | |
| func (s *Service) Login(ctx context.Context, username, password string) (string, error) { | |
| user, err := s.queries.GetUserByUsername(ctx, username) | |
| if err != nil { | |
| if errors.Is(err, pgx.ErrNoRows) { | |
| // Perform a dummy check to equalize timing with the real-password path | |
| users.CheckPassword(password, dummyHash) | |
| return "", ErrInvalidCredentials | |
| } | |
| return "", fmt.Errorf("query user: %w", err) | |
| } | |
| if !users.CheckPassword(password, user.PasswordHash) { | |
| return "", ErrInvalidCredentials | |
| } |
🤖 Prompt for AI Agents
In `@internal/auth/service.go` around lines 63 - 74, The Login function leaks
username existence via timing: when s.queries.GetUserByUsername returns
pgx.ErrNoRows you return immediately but when a user exists you call
users.CheckPassword which invokes bcrypt and takes much longer; to fix, in the
GetUserByUsername error path perform a dummy bcrypt comparison using a constant
precomputed hash (e.g., call bcrypt.CompareHashAndPassword on a package-level
fakeHash) before returning ErrInvalidCredentials so both branches execute a slow
bcrypt operation, keeping symbols to modify: Service.Login,
s.queries.GetUserByUsername, users.CheckPassword/ErrInvalidCredentials and
introduce a package-level fakeHash used for the dummy compare.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/api/http/handler/user.go`:
- Around line 23-37: In DeleteUser, avoid the unchecked type assertion on
c.Get("user_id") by capturing the second return value and validating the type
before calling h.userService.DeleteUser: call userVal, ok := c.Get("user_id");
if !ok { log and return an appropriate error response (e.g., 401 Unauthorized or
400 Bad Request) }, then strUserID, ok := userVal.(string); if !ok { log and
return an error response }, and only then call
h.userService.DeleteUser(c.Request.Context(), strUserID); ensure logs reference
the missing/invalid user_id and preserve existing error handling for
users.ErrUserNotFound and internal errors.
In `@internal/db/sqlc/users.sql.go`:
- Around line 51-58: The DeleteUser SQL currently uses the :exec directive
(deleteUser) so Queries.DeleteUser cannot tell if no row was deleted; update the
source SQL to use :execrows or :execresult so the generated code returns rows
affected, then modify Queries.DeleteUser to check the command/result for
rowsAffected == 0 and return a not-found error (e.g., sql.ErrNoRows or your
project's NotFound error) instead of silently returning nil; keep the deleteUser
SQL name and the DeleteUser method signature intact while adding the
rows-affected check and appropriate error return.
In `@internal/users/service.go`:
- Around line 31-42: DeleteUser currently returns nil for non-existent DB rows
because s.queries.DeleteUser uses a :exec SQL that doesn't surface rows
affected; only malformed UUIDs return ErrUserNotFound. Fix by changing the SQL
to :execrows so sqlc returns an ExecResult (or modify the call site to capture
the ExecResult) and check ExecResult.RowsAffected(); if RowsAffected() == 0
return ErrUserNotFound, otherwise proceed. Update DeleteUser to parse uuid
(uuid.Parse/pgtype.UUID) as before, call the queries delete method that returns
ExecResult, and return ErrUserNotFound when rows affected is zero.
🧹 Nitpick comments (6)
internal/api/http/dto/auth.go (1)
5-5: Consider adding amaxconstraint on the password field.Without an upper bound, an extremely long password payload will still be processed by bcrypt. bcrypt itself truncates input at 72 bytes, so anything beyond that is wasted work and memory. A reasonable
max=128(or similar) prevents abuse while remaining user-friendly.The same applies to
LoginRequest.Passwordon Line 16.Proposed fix
- Password string `json:"password" binding:"required,min=8"` + Password string `json:"password" binding:"required,min=8,max=128"`systemtest/tests/auth.go (2)
172-189: Silently discardingjson.Marshalerror in test helpers.
json.Marshalon Lines 173 and 182 can't realistically fail for these DTO structs, but swallowing errors in test code can mask unexpected issues. Consider using a*testing.Tparameter withrequire.Proposed fix (example for doJSON)
-func doJSON(router *gin.Engine, method, path string, body any) *httptest.ResponseRecorder { - b, _ := json.Marshal(body) +func doJSON(router *gin.Engine, method, path string, body any) *httptest.ResponseRecorder { + b, err := json.Marshal(body) + if err != nil { + panic(fmt.Sprintf("doJSON: failed to marshal body: %v", err)) + }
118-120: Hardcoded admin credentials in test.
"root"/"changeme"on Line 120 assumes the test harness seeds this admin user. If the seed data changes, this test silently breaks. Consider extracting these into shared test constants alongside the test setup that creates them.internal/api/http/middleware/auth.go (1)
32-34: Consider using typed context keys to avoid collision with other middleware.String keys like
"user_id","username","role"risk collision if other middleware or handlers use the same keys. Defining package-level constants (or a custom type) makes this more robust and easier to refactor.Example
const ( CtxKeyUserID = "user_id" CtxKeyUsername = "username" CtxKeyRole = "role" )internal/users/service.go (2)
70-73: Preferuuid.UUID(id).String()over customuuidToString.You already import
github.com/google/uuid. ItsUUIDtype is[16]byteand has a well-testedString()method, so you can replace this helper with:uuid.UUID(u.ID.Bytes).String()This avoids subtle formatting bugs (e.g., the current
%012xformat would drop leading zeros in the last segment if Go'sfmtbehaved unexpectedly, though in practice it works).♻️ Proposed diff
-func uuidToString(id [16]byte) string { - return fmt.Sprintf("%08x-%04x-%04x-%04x-%012x", - id[0:4], id[4:6], id[6:8], id[8:10], id[10:16]) -}And update usage at Line 61:
- ID: uuidToString(u.ID.Bytes), + ID: uuid.UUID(u.ID.Bytes).String(),
44-68: Minor:CountUsersis a separate query, introducing a TOCTOU gap.The total count and the paginated list are fetched in two separate queries (Lines 45 and 53). Between the two calls, a user could be created or deleted, making
totalinconsistent with the returned page of users. This is generally acceptable for pagination UIs but worth noting. If strict consistency is needed, both could be done in a single transaction.
| func (h *UserHandler) DeleteUser(c *gin.Context) { | ||
| userID, _ := c.Get("user_id") | ||
|
|
||
| if err := h.userService.DeleteUser(c.Request.Context(), userID.(string)); err != nil { | ||
| if errors.Is(err, users.ErrUserNotFound) { | ||
| c.JSON(http.StatusNotFound, gin.H{"error": "user not found"}) | ||
| return | ||
| } | ||
| slog.Error("Failed to delete user", "error", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "internal error"}) | ||
| return | ||
| } | ||
|
|
||
| c.Status(http.StatusNoContent) | ||
| } |
There was a problem hiding this comment.
Unchecked type assertion on user_id can panic.
c.Get("user_id") returns (interface{}, bool). If the key is missing or the value isn't a string, the bare assertion userID.(string) on Line 26 will panic at runtime. While the JWT middleware should always set this, a defensive check prevents a crash from misconfiguration or middleware ordering bugs.
🛡️ Proposed fix
func (h *UserHandler) DeleteUser(c *gin.Context) {
- userID, _ := c.Get("user_id")
+ userIDVal, exists := c.Get("user_id")
+ if !exists {
+ c.JSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"})
+ return
+ }
+ userID, ok := userIDVal.(string)
+ if !ok {
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "internal error"})
+ return
+ }
- if err := h.userService.DeleteUser(c.Request.Context(), userID.(string)); err != nil {
+ if err := h.userService.DeleteUser(c.Request.Context(), userID); err != nil {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (h *UserHandler) DeleteUser(c *gin.Context) { | |
| userID, _ := c.Get("user_id") | |
| if err := h.userService.DeleteUser(c.Request.Context(), userID.(string)); err != nil { | |
| if errors.Is(err, users.ErrUserNotFound) { | |
| c.JSON(http.StatusNotFound, gin.H{"error": "user not found"}) | |
| return | |
| } | |
| slog.Error("Failed to delete user", "error", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "internal error"}) | |
| return | |
| } | |
| c.Status(http.StatusNoContent) | |
| } | |
| func (h *UserHandler) DeleteUser(c *gin.Context) { | |
| userIDVal, exists := c.Get("user_id") | |
| if !exists { | |
| c.JSON(http.StatusUnauthorized, gin.H{"error": "unauthorized"}) | |
| return | |
| } | |
| userID, ok := userIDVal.(string) | |
| if !ok { | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "internal error"}) | |
| return | |
| } | |
| if err := h.userService.DeleteUser(c.Request.Context(), userID); err != nil { | |
| if errors.Is(err, users.ErrUserNotFound) { | |
| c.JSON(http.StatusNotFound, gin.H{"error": "user not found"}) | |
| return | |
| } | |
| slog.Error("Failed to delete user", "error", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "internal error"}) | |
| return | |
| } | |
| c.Status(http.StatusNoContent) | |
| } |
🤖 Prompt for AI Agents
In `@internal/api/http/handler/user.go` around lines 23 - 37, In DeleteUser, avoid
the unchecked type assertion on c.Get("user_id") by capturing the second return
value and validating the type before calling h.userService.DeleteUser: call
userVal, ok := c.Get("user_id"); if !ok { log and return an appropriate error
response (e.g., 401 Unauthorized or 400 Bad Request) }, then strUserID, ok :=
userVal.(string); if !ok { log and return an error response }, and only then
call h.userService.DeleteUser(c.Request.Context(), strUserID); ensure logs
reference the missing/invalid user_id and preserve existing error handling for
users.ErrUserNotFound and internal errors.
| const deleteUser = `-- name: DeleteUser :exec | ||
| DELETE FROM users WHERE id = $1 | ||
| ` | ||
|
|
||
| func (q *Queries) DeleteUser(ctx context.Context, id pgtype.UUID) error { | ||
| _, err := q.db.Exec(ctx, deleteUser, id) | ||
| return err | ||
| } |
There was a problem hiding this comment.
DeleteUser :exec doesn't report whether a row was actually deleted.
As noted in the internal/users/service.go review, this query silently succeeds even when no row matches. To enable proper "not found" detection, consider changing the query to :execrows or :execresult in the source SQL file (internal/db/queries/users.sql).
🤖 Prompt for AI Agents
In `@internal/db/sqlc/users.sql.go` around lines 51 - 58, The DeleteUser SQL
currently uses the :exec directive (deleteUser) so Queries.DeleteUser cannot
tell if no row was deleted; update the source SQL to use :execrows or
:execresult so the generated code returns rows affected, then modify
Queries.DeleteUser to check the command/result for rowsAffected == 0 and return
a not-found error (e.g., sql.ErrNoRows or your project's NotFound error) instead
of silently returning nil; keep the deleteUser SQL name and the DeleteUser
method signature intact while adding the rows-affected check and appropriate
error return.
| func (s *Service) DeleteUser(ctx context.Context, userID string) error { | ||
| parsed, err := uuid.Parse(userID) | ||
| if err != nil { | ||
| return ErrUserNotFound | ||
| } | ||
|
|
||
| pgID := pgtype.UUID{Bytes: parsed, Valid: true} | ||
| if err := s.queries.DeleteUser(ctx, pgID); err != nil { | ||
| return fmt.Errorf("delete user: %w", err) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "*.sql" -type f | head -20Repository: EternisAI/silo-proxy
Length of output: 142
🏁 Script executed:
fd -e sql -type f | xargs grep -l "DeleteUser"Repository: EternisAI/silo-proxy
Length of output: 236
🏁 Script executed:
find . -path "*/internal/*" -name "*.go" | xargs grep -l "DeleteUser" | head -10Repository: EternisAI/silo-proxy
Length of output: 219
🏁 Script executed:
cat ./internal/db/queries/users.sqlRepository: EternisAI/silo-proxy
Length of output: 534
🏁 Script executed:
cat ./internal/api/http/handler/user.goRepository: EternisAI/silo-proxy
Length of output: 1842
🏁 Script executed:
cat ./internal/db/sqlc/users.sql.goRepository: EternisAI/silo-proxy
Length of output: 3018
DeleteUser returns success (204) for missing users instead of detecting and returning an error for 404.
The SQL query is :exec, which in sqlc generates code that only checks for query errors, not rows affected. When a DELETE executes for a non-existent user ID, PostgreSQL succeeds with 0 rows affected and returns no error. The service then returns nil, and the handler returns 204 No Content. ErrUserNotFound is only returned for malformed UUID strings (line 34), not for missing users in the database. However, the handler at line 27 checks for this error to return a 404, which will never trigger for the actual "user not in DB" case.
Fix by either switching to :execrows in the SQL (which returns rows affected) or capturing the ExecResult from the current :exec and checking RowsAffected() == 0 before returning nil.
🤖 Prompt for AI Agents
In `@internal/users/service.go` around lines 31 - 42, DeleteUser currently returns
nil for non-existent DB rows because s.queries.DeleteUser uses a :exec SQL that
doesn't surface rows affected; only malformed UUIDs return ErrUserNotFound. Fix
by changing the SQL to :execrows so sqlc returns an ExecResult (or modify the
call site to capture the ExecResult) and check ExecResult.RowsAffected(); if
RowsAffected() == 0 return ErrUserNotFound, otherwise proceed. Update DeleteUser
to parse uuid (uuid.Parse/pgtype.UUID) as before, call the queries delete method
that returns ExecResult, and return ErrUserNotFound when rows affected is zero.
1 participant
Summary by CodeRabbit