From 6202b6e03eddfe9a8e2974c88bb6fcab9f2dd8de Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Tue, 20 Jan 2026 15:24:47 -1000 Subject: Add workflow documentation for auth implementation Include surgical instructions and code review feedback from authentication feature development. Co-Authored-By: Claude Opus 4.5 --- review_feedback.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 review_feedback.md (limited to 'review_feedback.md') 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. -- cgit v1.2.3