fix: replace log.Printf with structured slog logging in auth and ratelimit modules#63
Conversation
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Ved, good instinct to standardize the logging — the codebase should be consistent on slog, and cleaning up stale log.Printf calls is worthwhile. There are a few things that need fixing before this can land.
Critical Issues (1 found)
main.go change conflicts with merged PR #59 and reverses a security fix
This PR replaces the // TODO: protect with requireAuth once auth lands comment with // intentionally public for health monitoring and leaves the /api/v1/admin/status endpoint unprotected. However, PR #59 (merged today) already resolved this TODO correctly — it wraps the endpoint with authMiddleware(adminMiddleware(...)), requiring both authentication and admin role.
This PR has a merge conflict (CONFLICTING state on GitHub), and if resolved by keeping this PR's version, it would revert a security fix by removing the auth protection on the admin status endpoint.
Fix: Rebase onto latest main and drop the main.go change entirely. PR #59 already handled this correctly.
Important Issues (2 found)
1. Two log messages have wrong descriptions
auth.go:74 logs "token validation failed" for what is actually a database error during GetUserByEmail. auth.go:85 logs the same "token validation failed" for what is actually a bcrypt internal error (not a password mismatch — that's handled on line 81). The original messages ("login: database error" and "login: bcrypt error") were more accurate.
When an engineer sees "token validation failed" in logs three times, they'll assume it's the same code path. But these are three different failure modes — a database error, a bcrypt library error, and an actual token validation failure — and they require different investigation steps.
Fix: Use descriptive messages that match the actual failure:
- Line 74:
slog.Error("login: database error", "error", err) - Line 85:
slog.Error("login: bcrypt error", "error", err) - Line 137:
slog.Warn("token validation failed", "error", err)(this one is correct)
2. Severity should be slog.Error for internal server errors, not slog.Warn
Lines 74 and 85 are error paths that return HTTP 500 — these represent genuine internal failures (database unreachable, bcrypt library malfunction). Similarly, line 92 ("token generation failed") returns 500. These should be slog.Error, not slog.Warn. Warnings are appropriate for client errors or degraded-but-functional states (like the rate limiter capacity warning in ratelimit.go, which is correct as slog.Warn).
The token validation failure on line 137 is correctly slog.Warn since it's a client-side error (bad token) returning 401, not a server failure.
Suggestions (0 found)
Strengths
- The
ratelimit.gochange is clean and correct — structured logging with"capacity"and"key"fields is a nice improvement over the format string - Removing the
"log"import in favor of"log/slog"is the right direction - Good local testing checklist in the PR description
Recommended Action
- Rebase onto latest
mainand drop themain.gochange (already handled by PR #59) - Fix the misleading log messages in
auth.goto match their actual error conditions - Use
slog.Errorinstead ofslog.Warnfor paths that return HTTP 500
Vedd-Patel
force-pushed
the
fix/auth-slog-cleanup
branch
from
618b141 to
9e1986c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplaced unstructured Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (1)
auth.go (1)
163-165: Optional: avoid emittingerror=nilon invalid token path.When
token.Validis false anderris nil, current logging records a null error field. Splitting branches makes logs clearer.♻️ Proposed refinement
- if err != nil || !token.Valid { - slog.Warn("token validation failed", "error", err) - writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid token"}) - return - } + if err != nil { + slog.Warn("token validation failed", "error", err) + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid token"}) + return + } + if !token.Valid { + slog.Warn("token validation failed: token marked invalid") + writeJSON(w, http.StatusUnauthorized, map[string]string{"error": "invalid token"}) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@auth.go` around lines 163 - 165, The log call should not emit a nil error when token.Valid is false; in the token validation branch (where token.Valid and err are checked) split the condition into two branches: if err != nil then call slog.Warn (or slog.Error) including the error value, else if token.Valid is false then call slog.Warn with a descriptive message but no "error" field; keep the existing writeJSON(w, http.StatusUnauthorized, ...) response in both branches so authentication failure behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@auth.go`:
- Around line 163-165: The log call should not emit a nil error when token.Valid
is false; in the token validation branch (where token.Valid and err are checked)
split the condition into two branches: if err != nil then call slog.Warn (or
slog.Error) including the error value, else if token.Valid is false then call
slog.Warn with a descriptive message but no "error" field; keep the existing
writeJSON(w, http.StatusUnauthorized, ...) response in both branches so
authentication failure behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab76bfb7-5da0-466c-a0a8-8e0dcee72fc8
📒 Files selected for processing (2)
auth.goratelimit.go
Vedd-Patel
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 `@auth.go`:
- Around line 163-173: Remove the stray diff marker '+' characters at the start
of the token validation guard clauses so the Go statements compile; edit the
block that checks err and token.Valid (the lines that call slog.Warn(...),
writeJSON(...), and return) to remove the leading '+' on each line so the code
reads as valid Go (refer to the token validation block using variables err,
token.Valid, slog.Warn, writeJSON, and w).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
aaronbrethorst
left a comment
aaronbrethorst
left a comment
There was a problem hiding this comment.
Ved, this is exactly the follow-through I was hoping for. Every item from the last round is handled, and the diff is tighter for it.
Prior Feedback — All Addressed
Critical: main.go conflict / security revert — resolved
The main.go change is dropped entirely. The PR no longer touches that file, git diff main...HEAD -- main.go is empty, and GitHub now reports MERGEABLE / CLEAN (was CONFLICTING). PR #59's auth protection on /api/v1/admin/status stays intact.
Important #1: Misleading log messages — fixed
auth.go:73→slog.Error("login: database error", "error", err)auth.go:84→slog.Error("login: bcrypt error", "error", err)auth.go:164/:170→"token validation failed"(correct — these are the genuine token-validation paths)
Each message now matches its actual failure mode.
Important #2: Severity — fixed
The three HTTP 500 paths (:73, :84, :91) are now slog.Error. The two 401 client-error paths in requireAuth (:164, :170) correctly stay slog.Warn, as does the rate-limiter capacity warning. Severity now lines up with whether the failure is server-side or client-side.
Critical Issues (0 found)
Important Issues (0 found)
Suggestions (0 found)
Strengths
- Splitting the old
if err != nil || !token.Validinto two branches (:163and:169) is a real readability win — the two failure modes now log distinctly instead of collapsing into one ambiguous line. ratelimit.gostructured logging with"capacity"and"key"attributes reads cleanly.- Dropping the
"log"import in favor of"log/slog"is the right direction for codebase consistency. - Verified locally:
go test ./...passes,go vet ./...clean,gofmt -lreports nothing.
Recommended Action
Ship it. No further changes needed.
2 participants
Description
Fixes #52
This PR addresses the minor housekeeping items identified during the authentication module review. It standardizes logging across the newly added auth files to match the rest of the codebase
Changes
auth.go: Replaced standardlog.Printfcalls with structuredslog.Warnandslog.Errorfor login failures and token validation errors. Removed the standardlogimport.ratelimit.go: Replaced standardlog.Printfcalls with structuredslog.Warnfor capacity warnings and rate limit triggers. Removed the standardlogimport.main.go: Replaced the stale// TODO: protect with requireAuth once auth landscomment above the/api/v1/admin/statusendpoint with// intentionally public for health monitoring, as requested.Local Testing
go fmt ./...go vet ./...go test ./... -v)Summary by CodeRabbit