From aa5ab6b4152bbc0d0cdc234f7fbc1bbb69a1c98d Mon Sep 17 00:00:00 2001 From: Ole-Morten Duesund Date: Sun, 29 Mar 2026 16:39:10 +0200 Subject: [PATCH] fix: address code review findings for Phase 7-8 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugs fixed: - Renderer.Error set WriteHeader before Content-Type, causing the header to be silently dropped. Now sets Content-Type first. - truncate template function operated on bytes, not runes — could split multi-byte UTF-8 characters (Norwegian æøå). Now uses []rune for correct Unicode handling. Performance: - Skip session DB lookup (2 queries) on /static/ and /uploads/ requests — these never use user context. UX consistency: - Replace all http.NotFound and http.Error("Forbidden") in handler layer with styled error pages via Renderer.Error. - Add notFound/forbidden helper methods on Handler. Deployment fixes: - Remove false libc6/glibc deps from nfpm.yaml (binary is statically linked with CGO_ENABLED=0). - Add CGO_ENABLED=0 to Makefile build target for consistency. - Add .dockerignore to exclude .git, dist/, data/ from build context. - Remove phantom 'lint' from Makefile .PHONY. - Document ProtectSystem=strict constraint in systemd service. Co-Authored-By: Claude Opus 4.6 (1M context) --- .dockerignore | 9 +++++++++ Makefile | 4 ++-- dist/favoritter.service | 5 ++++- internal/handler/fave.go | 24 ++++++++++++------------ internal/handler/handler.go | 9 +++++++-- internal/handler/profile.go | 4 ++-- internal/middleware/auth.go | 10 ++++++++++ internal/render/render.go | 32 ++++++++++++++++++++++++++------ nfpm.yaml | 8 +------- 9 files changed, 73 insertions(+), 32 deletions(-) create mode 100644 .dockerignore diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..c27042d --- /dev/null +++ b/.dockerignore @@ -0,0 +1,9 @@ +.git +dist/ +data/ +*.db +*.sqlite +.env +*.env.local +.vscode +.idea diff --git a/Makefile b/Makefile index a39b8a4..2c64c94 100644 --- a/Makefile +++ b/Makefile @@ -7,11 +7,11 @@ LDFLAGS := -s -w -X main.version=$(VERSION) -X main.buildDate=$(BUILD_DATE) PLATFORMS := linux/amd64 linux/arm64 DIST := dist -.PHONY: build build-all deb rpm container artifacts checksums clean test lint +.PHONY: build build-all deb rpm container tarballs artifacts checksums clean test ## Build for current platform build: - go build -ldflags="$(LDFLAGS)" -o favoritter ./cmd/favoritter + CGO_ENABLED=0 go build -ldflags="$(LDFLAGS)" -o favoritter ./cmd/favoritter ## Run tests test: diff --git a/dist/favoritter.service b/dist/favoritter.service index 6de8f51..22a5c54 100644 --- a/dist/favoritter.service +++ b/dist/favoritter.service @@ -11,7 +11,10 @@ ExecStart=/usr/bin/favoritter Restart=on-failure RestartSec=5 -# Hardening +# Hardening — ProtectSystem=strict makes the filesystem read-only +# except for ReadWritePaths. If you change FAVORITTER_UPLOAD_DIR or +# FAVORITTER_DB_PATH to a location outside /var/lib/favoritter, add +# that path to ReadWritePaths. NoNewPrivileges=yes ProtectSystem=strict ProtectHome=yes diff --git a/internal/handler/fave.go b/internal/handler/fave.go index a739370..220093c 100644 --- a/internal/handler/fave.go +++ b/internal/handler/fave.go @@ -125,14 +125,14 @@ func (h *Handler) handleFaveCreate(w http.ResponseWriter, r *http.Request) { func (h *Handler) handleFaveDetail(w http.ResponseWriter, r *http.Request) { id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) if err != nil { - http.NotFound(w, r) + h.notFound(w, r) return } fave, err := h.deps.Faves.GetByID(id) if err != nil { if errors.Is(err, store.ErrFaveNotFound) { - http.NotFound(w, r) + h.notFound(w, r) return } slog.Error("get fave error", "error", err) @@ -143,7 +143,7 @@ func (h *Handler) handleFaveDetail(w http.ResponseWriter, r *http.Request) { // Check access: private faves are only visible to their owner. user := middleware.UserFromContext(r.Context()) if fave.Privacy == "private" && (user == nil || user.ID != fave.UserID) { - http.NotFound(w, r) + h.notFound(w, r) return } @@ -169,14 +169,14 @@ func (h *Handler) handleFaveEdit(w http.ResponseWriter, r *http.Request) { id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) if err != nil { - http.NotFound(w, r) + h.notFound(w, r) return } fave, err := h.deps.Faves.GetByID(id) if err != nil { if errors.Is(err, store.ErrFaveNotFound) { - http.NotFound(w, r) + h.notFound(w, r) return } slog.Error("get fave error", "error", err) @@ -186,7 +186,7 @@ func (h *Handler) handleFaveEdit(w http.ResponseWriter, r *http.Request) { // Only the owner can edit. if user.ID != fave.UserID { - http.Error(w, "Forbidden", http.StatusForbidden) + h.forbidden(w, r) return } @@ -217,14 +217,14 @@ func (h *Handler) handleFaveUpdate(w http.ResponseWriter, r *http.Request) { id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) if err != nil { - http.NotFound(w, r) + h.notFound(w, r) return } fave, err := h.deps.Faves.GetByID(id) if err != nil { if errors.Is(err, store.ErrFaveNotFound) { - http.NotFound(w, r) + h.notFound(w, r) return } slog.Error("get fave error", "error", err) @@ -233,7 +233,7 @@ func (h *Handler) handleFaveUpdate(w http.ResponseWriter, r *http.Request) { } if user.ID != fave.UserID { - http.Error(w, "Forbidden", http.StatusForbidden) + h.forbidden(w, r) return } @@ -304,14 +304,14 @@ func (h *Handler) handleFaveDelete(w http.ResponseWriter, r *http.Request) { id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) if err != nil { - http.NotFound(w, r) + h.notFound(w, r) return } fave, err := h.deps.Faves.GetByID(id) if err != nil { if errors.Is(err, store.ErrFaveNotFound) { - http.NotFound(w, r) + h.notFound(w, r) return } slog.Error("get fave error", "error", err) @@ -320,7 +320,7 @@ func (h *Handler) handleFaveDelete(w http.ResponseWriter, r *http.Request) { } if user.ID != fave.UserID { - http.Error(w, "Forbidden", http.StatusForbidden) + h.forbidden(w, r) return } diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 6fcea29..2b5f879 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -144,7 +144,12 @@ func (h *Handler) Routes() *http.ServeMux { return mux } -// handleNotFound renders a styled 404 page. -func (h *Handler) handleNotFound(w http.ResponseWriter, r *http.Request) { +// notFound renders a styled 404 error page. +func (h *Handler) notFound(w http.ResponseWriter, r *http.Request) { h.deps.Renderer.Error(w, r, http.StatusNotFound, "Ikke funnet") } + +// forbidden renders a styled 403 error page. +func (h *Handler) forbidden(w http.ResponseWriter, r *http.Request) { + h.deps.Renderer.Error(w, r, http.StatusForbidden, "Ingen tilgang") +} diff --git a/internal/handler/profile.go b/internal/handler/profile.go index 7c520f3..caf2e9c 100644 --- a/internal/handler/profile.go +++ b/internal/handler/profile.go @@ -22,7 +22,7 @@ func (h *Handler) handlePublicProfile(w http.ResponseWriter, r *http.Request) { profileUser, err := h.deps.Users.GetByUsername(username) if err != nil { if errors.Is(err, store.ErrUserNotFound) { - http.NotFound(w, r) + h.notFound(w, r) return } slog.Error("get user error", "error", err) @@ -31,7 +31,7 @@ func (h *Handler) handlePublicProfile(w http.ResponseWriter, r *http.Request) { } if profileUser.Disabled { - http.NotFound(w, r) + h.notFound(w, r) return } diff --git a/internal/middleware/auth.go b/internal/middleware/auth.go index 54c13a6..7cea7d0 100644 --- a/internal/middleware/auth.go +++ b/internal/middleware/auth.go @@ -6,6 +6,7 @@ import ( "context" "errors" "net/http" + "strings" "kode.naiv.no/olemd/favoritter/internal/store" ) @@ -28,6 +29,15 @@ func ClearSessionCookie(w http.ResponseWriter) { func SessionLoader(sessions *store.SessionStore, users *store.UserStore) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Skip session lookup for static assets and uploads — they + // never use the user context and this avoids 2 DB queries + // per asset per page load. + if strings.HasPrefix(r.URL.Path, "/static/") || + strings.HasPrefix(r.URL.Path, "/uploads/") { + next.ServeHTTP(w, r) + return + } + cookie, err := r.Cookie(SessionCookieName) if err != nil { next.ServeHTTP(w, r) diff --git a/internal/render/render.go b/internal/render/render.go index b423f03..d91ad1a 100644 --- a/internal/render/render.go +++ b/internal/render/render.go @@ -154,14 +154,32 @@ func (r *Renderer) Page(w http.ResponseWriter, req *http.Request, name string, d // Error renders an error page with the given HTTP status code. func (r *Renderer) Error(w http.ResponseWriter, req *http.Request, code int, message string) { - w.WriteHeader(code) - r.Page(w, req, "error", PageData{ + data := PageData{ Title: message, Data: map[string]any{ "Code": code, "Message": message, }, - }) + } + + // Populate common fields from context (same as Page does). + data.User = middleware.UserFromContext(req.Context()) + data.CSRFToken = middleware.CSRFTokenFromContext(req.Context()) + data.BasePath = r.cfg.BasePath + data.SiteName = r.cfg.SiteName + data.ExternalURL = r.cfg.ExternalURL + + t, ok := r.templates["error"] + if !ok { + http.Error(w, message, code) + return + } + + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.WriteHeader(code) + if err := t.ExecuteTemplate(w, "base.html", data); err != nil { + slog.Error("error template execute failed", "error", err) + } } // Partial renders a partial template (for HTMX responses). @@ -184,12 +202,14 @@ func (r *Renderer) templateFuncs() template.FuncMap { "externalURL": func() string { return r.cfg.ExternalURL }, - // truncate truncates a string to n characters. + // truncate truncates a string to n runes (not bytes) to avoid + // splitting multi-byte UTF-8 characters (Norwegian æøå etc.). "truncate": func(n int, s string) string { - if len(s) <= n { + runes := []rune(s) + if len(runes) <= n { return s } - return s[:n] + "..." + return string(runes[:n]) + "..." }, // join joins strings with a separator. "join": strings.Join, diff --git a/nfpm.yaml b/nfpm.yaml index ceb1fd9..d394703 100644 --- a/nfpm.yaml +++ b/nfpm.yaml @@ -43,10 +43,4 @@ scripts: postinstall: ./dist/postinstall.sh preremove: ./dist/preremove.sh -overrides: - deb: - depends: - - libc6 - rpm: - depends: - - glibc + # No runtime dependencies — binary is statically linked (CGO_ENABLED=0).