fix: address code review findings for Phase 7-8

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) <noreply@anthropic.com>
This commit is contained in:
Ole-Morten Duesund 2026-03-29 16:39:10 +02:00
commit aa5ab6b415
9 changed files with 73 additions and 32 deletions

9
.dockerignore Normal file
View file

@ -0,0 +1,9 @@
.git
dist/
data/
*.db
*.sqlite
.env
*.env.local
.vscode
.idea

View file

@ -7,11 +7,11 @@ LDFLAGS := -s -w -X main.version=$(VERSION) -X main.buildDate=$(BUILD_DATE)
PLATFORMS := linux/amd64 linux/arm64 PLATFORMS := linux/amd64 linux/arm64
DIST := dist 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 for current platform
build: build:
go build -ldflags="$(LDFLAGS)" -o favoritter ./cmd/favoritter CGO_ENABLED=0 go build -ldflags="$(LDFLAGS)" -o favoritter ./cmd/favoritter
## Run tests ## Run tests
test: test:

View file

@ -11,7 +11,10 @@ ExecStart=/usr/bin/favoritter
Restart=on-failure Restart=on-failure
RestartSec=5 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 NoNewPrivileges=yes
ProtectSystem=strict ProtectSystem=strict
ProtectHome=yes ProtectHome=yes

View file

@ -125,14 +125,14 @@ func (h *Handler) handleFaveCreate(w http.ResponseWriter, r *http.Request) {
func (h *Handler) handleFaveDetail(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) id, err := strconv.ParseInt(r.PathValue("id"), 10, 64)
if err != nil { if err != nil {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
fave, err := h.deps.Faves.GetByID(id) fave, err := h.deps.Faves.GetByID(id)
if err != nil { if err != nil {
if errors.Is(err, store.ErrFaveNotFound) { if errors.Is(err, store.ErrFaveNotFound) {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
slog.Error("get fave error", "error", err) 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. // Check access: private faves are only visible to their owner.
user := middleware.UserFromContext(r.Context()) user := middleware.UserFromContext(r.Context())
if fave.Privacy == "private" && (user == nil || user.ID != fave.UserID) { if fave.Privacy == "private" && (user == nil || user.ID != fave.UserID) {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
@ -169,14 +169,14 @@ func (h *Handler) handleFaveEdit(w http.ResponseWriter, r *http.Request) {
id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) id, err := strconv.ParseInt(r.PathValue("id"), 10, 64)
if err != nil { if err != nil {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
fave, err := h.deps.Faves.GetByID(id) fave, err := h.deps.Faves.GetByID(id)
if err != nil { if err != nil {
if errors.Is(err, store.ErrFaveNotFound) { if errors.Is(err, store.ErrFaveNotFound) {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
slog.Error("get fave error", "error", err) 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. // Only the owner can edit.
if user.ID != fave.UserID { if user.ID != fave.UserID {
http.Error(w, "Forbidden", http.StatusForbidden) h.forbidden(w, r)
return return
} }
@ -217,14 +217,14 @@ func (h *Handler) handleFaveUpdate(w http.ResponseWriter, r *http.Request) {
id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) id, err := strconv.ParseInt(r.PathValue("id"), 10, 64)
if err != nil { if err != nil {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
fave, err := h.deps.Faves.GetByID(id) fave, err := h.deps.Faves.GetByID(id)
if err != nil { if err != nil {
if errors.Is(err, store.ErrFaveNotFound) { if errors.Is(err, store.ErrFaveNotFound) {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
slog.Error("get fave error", "error", err) 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 { if user.ID != fave.UserID {
http.Error(w, "Forbidden", http.StatusForbidden) h.forbidden(w, r)
return return
} }
@ -304,14 +304,14 @@ func (h *Handler) handleFaveDelete(w http.ResponseWriter, r *http.Request) {
id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) id, err := strconv.ParseInt(r.PathValue("id"), 10, 64)
if err != nil { if err != nil {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
fave, err := h.deps.Faves.GetByID(id) fave, err := h.deps.Faves.GetByID(id)
if err != nil { if err != nil {
if errors.Is(err, store.ErrFaveNotFound) { if errors.Is(err, store.ErrFaveNotFound) {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
slog.Error("get fave error", "error", err) 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 { if user.ID != fave.UserID {
http.Error(w, "Forbidden", http.StatusForbidden) h.forbidden(w, r)
return return
} }

View file

@ -144,7 +144,12 @@ func (h *Handler) Routes() *http.ServeMux {
return mux return mux
} }
// handleNotFound renders a styled 404 page. // notFound renders a styled 404 error page.
func (h *Handler) handleNotFound(w http.ResponseWriter, r *http.Request) { func (h *Handler) notFound(w http.ResponseWriter, r *http.Request) {
h.deps.Renderer.Error(w, r, http.StatusNotFound, "Ikke funnet") 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")
}

View file

@ -22,7 +22,7 @@ func (h *Handler) handlePublicProfile(w http.ResponseWriter, r *http.Request) {
profileUser, err := h.deps.Users.GetByUsername(username) profileUser, err := h.deps.Users.GetByUsername(username)
if err != nil { if err != nil {
if errors.Is(err, store.ErrUserNotFound) { if errors.Is(err, store.ErrUserNotFound) {
http.NotFound(w, r) h.notFound(w, r)
return return
} }
slog.Error("get user error", "error", err) slog.Error("get user error", "error", err)
@ -31,7 +31,7 @@ func (h *Handler) handlePublicProfile(w http.ResponseWriter, r *http.Request) {
} }
if profileUser.Disabled { if profileUser.Disabled {
http.NotFound(w, r) h.notFound(w, r)
return return
} }

View file

@ -6,6 +6,7 @@ import (
"context" "context"
"errors" "errors"
"net/http" "net/http"
"strings"
"kode.naiv.no/olemd/favoritter/internal/store" "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 { func SessionLoader(sessions *store.SessionStore, users *store.UserStore) func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { 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) cookie, err := r.Cookie(SessionCookieName)
if err != nil { if err != nil {
next.ServeHTTP(w, r) next.ServeHTTP(w, r)

View file

@ -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. // 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) { func (r *Renderer) Error(w http.ResponseWriter, req *http.Request, code int, message string) {
w.WriteHeader(code) data := PageData{
r.Page(w, req, "error", PageData{
Title: message, Title: message,
Data: map[string]any{ Data: map[string]any{
"Code": code, "Code": code,
"Message": message, "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). // Partial renders a partial template (for HTMX responses).
@ -184,12 +202,14 @@ func (r *Renderer) templateFuncs() template.FuncMap {
"externalURL": func() string { "externalURL": func() string {
return r.cfg.ExternalURL 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 { "truncate": func(n int, s string) string {
if len(s) <= n { runes := []rune(s)
if len(runes) <= n {
return s return s
} }
return s[:n] + "..." return string(runes[:n]) + "..."
}, },
// join joins strings with a separator. // join joins strings with a separator.
"join": strings.Join, "join": strings.Join,

View file

@ -43,10 +43,4 @@ scripts:
postinstall: ./dist/postinstall.sh postinstall: ./dist/postinstall.sh
preremove: ./dist/preremove.sh preremove: ./dist/preremove.sh
overrides: # No runtime dependencies — binary is statically linked (CGO_ENABLED=0).
deb:
depends:
- libc6
rpm:
depends:
- glibc