summaryrefslogtreecommitdiff
path: root/review_feedback.md
diff options
context:
space:
mode:
Diffstat (limited to 'review_feedback.md')
-rw-r--r--review_feedback.md29
1 files changed, 29 insertions, 0 deletions
diff --git a/review_feedback.md b/review_feedback.md
new file mode 100644
index 0000000..7ac8a0d
--- /dev/null
+++ b/review_feedback.md
@@ -0,0 +1,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.