blob: 7ac8a0d3fd65398d4b92c5bb3d1a214f5c13935e (
plain)
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
|
# Review Cycle 2025-05-20
## Status: [NEEDS_FIX]
## Critical Issues (Blocking)
1. **Missing Tests for Auth Package:**
* The `internal/auth` package has no unit tests.
* **Action:** Create `internal/auth/auth_test.go` and `internal/auth/handlers_test.go` to verify logic (hashing, session management, etc.).
2. **Missing CSRF Protection:**
* The login form uses POST but lacks a CSRF token. While `SameSite=Lax` helps, it is not a complete defense.
* **Action:** Integrate a CSRF middleware (e.g., `justinas/nosurf` or similar) or manually implement CSRF token verification for state-changing requests.
3. **Acceptance Tests Outdated:**
* `test/acceptance_test.go` sets up the server manually and bypasses the new authentication middleware.
* **Action:** Update `setupTestServer` in `test/acceptance_test.go` to include the auth middleware and mock the session/auth flow so tests can pass with the new security layer.
## Clean Code Suggestions (Non-Blocking)
1. **File Renaming:**
* `cmd/dashboard_main.go` was correctly identified as needing a rename. I have performed this rename to `cmd/dashboard/main.go`.
* **Action:** Ensure future edits target `cmd/dashboard/main.go`.
## Praise
* **Solid Auth Implementation:** The `internal/auth` package is well-structured and easy to read.
* **Secure Defaults:** Good use of `bcrypt` for hashing and `scs` for session management with `RenewToken` to prevent session fixation.
* **Clean Architecture:** The separation of concerns between Service, Handlers, and Middleware is excellent.
|
