From afd2476f3ce176c24ccd4d874ea5e712e987b87e Mon Sep 17 00:00:00 2001 From: codegirl-007 Date: Sat, 22 Aug 2026 07:24:39 -0700 Subject: [PATCH] Harden sessions, uploads, admin demotion, and HTTP timeouts. Address PR review findings: renew session tokens on auth, sniff/re-encode avatars, serialize last-admin checks, bound server timeouts, rune-safe truncation, and TEST_DATABASE_URL-only integration tests. --- .env.example | 2 + cmd/server/main.go | 9 +++- go.mod | 1 + go.sum | 2 + internal/store/sessions_test.go | 5 +-- internal/store/user.go | 8 ++++ internal/web/auth.go | 8 ++++ internal/web/profile.go | 73 ++++++++++++++++++++++++--------- internal/web/server.go | 23 +++++++++-- internal/web/server_test.go | 16 ++++---- todo.md | 2 +- 11 files changed, 113 insertions(+), 36 deletions(-) diff --git a/.env.example b/.env.example index a4d043c..ec4d4f2 100644 --- a/.env.example +++ b/.env.example @@ -3,6 +3,8 @@ LISTEN=:8080 # Required: PlanetScale Postgres URI (port 5432 so the app can create tables on boot). # Switch to 6432 (PgBouncer) later if you need pooling. DATABASE_URL=postgresql://user:password@host.example.com:5432/postgres?sslmode=verify-full +# Required for integration tests (do not point at the runtime DATABASE_URL). +# TEST_DATABASE_URL=postgresql://user:password@host.example.com:5432/plumber_test?sslmode=verify-full # Optional: first matching registrant becomes admin only if no admin exists yet. # Later promote/demote via /admin/users (admins only). ADMIN_USERNAME=yourusername diff --git a/cmd/server/main.go b/cmd/server/main.go index aed2530..b237b53 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -29,7 +29,14 @@ func main() { uploader := blob.FromEnv() handler := newHandler(db, sessions, uploader) - run(&http.Server{Addr: listenAddr(), Handler: handler}) + run(&http.Server{ + Addr: listenAddr(), + Handler: handler, + ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 60 * time.Second, + IdleTimeout: 90 * time.Second, + }) } func openDB() (*sql.DB, *store.SessionStore) { diff --git a/go.mod b/go.mod index 844fc09..ccba0df 100644 --- a/go.mod +++ b/go.mod @@ -12,6 +12,7 @@ require ( github.com/jackc/pgx/v5 v5.10.0 github.com/joho/godotenv v1.5.1 golang.org/x/crypto v0.55.0 + golang.org/x/image v0.45.0 ) require ( diff --git a/go.sum b/go.sum index 256af28..0a8c3d7 100644 --- a/go.sum +++ b/go.sum @@ -50,6 +50,8 @@ github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= golang.org/x/crypto v0.55.0 h1:+KWHjbgOaAQ66dh/YlkZKHlz9ZUlq61AFirAR9ntP8M= golang.org/x/crypto v0.55.0/go.mod h1:uq0V9dE/fzQuJtbnL+2EhWOE63vo164FY8xqEnV9xis= +golang.org/x/image v0.45.0 h1:FMb1nTbH5H9vF55SriQHgFw5GnNL9Jg6L25BwXKzhB0= +golang.org/x/image v0.45.0/go.mod h1:n62x/7RqlwXDvGsSU4u6IUTUf6KghUZ9Bt7cG/T9Fx4= golang.org/x/sync v0.22.0 h1:SZjpbeLmrCk4xhRSZFNZW5gFUeCeFgjekvI/+gfScek= golang.org/x/sync v0.22.0/go.mod h1:9xrNwdLfx4jkKbNva9FpL6vEN7evnE43NNNJQ2LF3+0= golang.org/x/text v0.41.0 h1:vz/seA0lnX87Othu2f/0L24RcgrXD9/YFTSuGjj3rH8= diff --git a/internal/store/sessions_test.go b/internal/store/sessions_test.go index b9430f7..d2caf4f 100644 --- a/internal/store/sessions_test.go +++ b/internal/store/sessions_test.go @@ -9,10 +9,7 @@ import ( func TestSessionStoreCommitFindDelete(t *testing.T) { url := os.Getenv("TEST_DATABASE_URL") if url == "" { - url = os.Getenv("DATABASE_URL") - } - if url == "" { - t.Skip("DATABASE_URL or TEST_DATABASE_URL not set") + t.Skip("TEST_DATABASE_URL not set") } schema, err := os.ReadFile("../../schema.sql") if err != nil { diff --git a/internal/store/user.go b/internal/store/user.go index 13bf795..7f4da01 100644 --- a/internal/store/user.go +++ b/internal/store/user.go @@ -92,6 +92,10 @@ func (u *User) Create(ctx context.Context) error { }) } +// adminRoleLockKey serializes SetRole so concurrent demotions cannot bypass the +// last-admin guard under READ COMMITTED. +const adminRoleLockKey int64 = 0x706c756d5f61646d // "plum_adm" + // SetRole updates this user's role (last-admin safe). func (u *User) SetRole(ctx context.Context, role Role) error { if u == nil || u.db == nil { @@ -106,6 +110,10 @@ func (u *User) SetRole(ctx context.Context, role Role) error { } defer tx.Rollback() + if _, err := tx.ExecContext(ctx, `SELECT pg_advisory_xact_lock($1)`, adminRoleLockKey); err != nil { + return err + } + q := sqlc.New(tx) current, err := q.GetUserRole(ctx, u.ID) if err != nil { diff --git a/internal/web/auth.go b/internal/web/auth.go index 5bf63d3..307900f 100644 --- a/internal/web/auth.go +++ b/internal/web/auth.go @@ -54,6 +54,10 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { }) return } + if err := s.sessions.RenewToken(r.Context()); err != nil { + http.Error(w, "could not start session", http.StatusInternalServerError) + return + } s.sessions.Put(r.Context(), "user_id", u.ID) http.Redirect(w, r, next, http.StatusSeeOther) } @@ -108,6 +112,10 @@ func (s *Server) handleRegister(w http.ResponseWriter, r *http.Request) { s.exec(w, "register", p) return } + if err := s.sessions.RenewToken(r.Context()); err != nil { + http.Error(w, "could not start session", http.StatusInternalServerError) + return + } s.sessions.Put(r.Context(), "user_id", u.ID) http.Redirect(w, r, "/", http.StatusSeeOther) } diff --git a/internal/web/profile.go b/internal/web/profile.go index fb9a750..9bf771b 100644 --- a/internal/web/profile.go +++ b/internal/web/profile.go @@ -1,12 +1,18 @@ package web import ( + "bytes" + "fmt" + "image" + "image/jpeg" + "image/png" "io" "net/http" "path" "strings" "github.com/google/uuid" + _ "golang.org/x/image/webp" "plumber/internal/blob" "plumber/internal/geo" @@ -63,23 +69,21 @@ func (s *Server) handleProfile(w http.ResponseWriter, r *http.Request) { s.renderProfile(w, r, u, "Avatar uploads are not configured on this server.", state) return } - ct := hdr.Header.Get("Content-Type") - ext, contentType, ok := avatarType(hdr.Filename, ct) - if !ok { - s.renderProfile(w, r, u, "Avatar must be a JPEG, PNG, or WebP image.", state) - return - } if hdr.Size > 2<<20 { s.renderProfile(w, r, u, "Avatar must be 2MB or smaller.", state) return } + body, ext, contentType, prepErr := prepareAvatar(file, 2<<20) + if prepErr != nil { + s.renderProfile(w, r, u, "Avatar must be a JPEG, PNG, or WebP image.", state) + return + } key := path.Join("avatars", u.ID, uuid.NewString()+ext) - limited := io.LimitReader(file, (2<<20)+1) url, upErr := s.cfg.Blob.Upload(r.Context(), blob.FileUpload{ Key: key, - Body: limited, + Body: bytes.NewReader(body), ContentType: contentType, - Size: hdr.Size, + Size: int64(len(body)), }) if upErr != nil { s.renderProfile(w, r, u, "Could not upload avatar. Try again later.", state) @@ -103,18 +107,49 @@ func (s *Server) handleProfile(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, "/profile", http.StatusSeeOther) } -func avatarType(filename, contentType string) (ext, normalized string, ok bool) { - contentType = strings.ToLower(strings.TrimSpace(contentType)) - filename = strings.ToLower(filename) +// prepareAvatar reads at most maxBytes, sniffs/decodes the image, and re-encodes +// it so only valid image bytes are stored publicly. +func prepareAvatar(r io.Reader, maxBytes int64) (body []byte, ext, contentType string, err error) { + limited := io.LimitReader(r, maxBytes+1) + raw, err := io.ReadAll(limited) + if err != nil { + return nil, "", "", err + } + if int64(len(raw)) > maxBytes { + return nil, "", "", fmt.Errorf("avatar too large") + } + if len(raw) == 0 { + return nil, "", "", fmt.Errorf("empty avatar") + } + + sniff := http.DetectContentType(raw) switch { - case strings.HasPrefix(contentType, "image/jpeg"), strings.HasSuffix(filename, ".jpg"), strings.HasSuffix(filename, ".jpeg"): - return ".jpg", "image/jpeg", true - case strings.HasPrefix(contentType, "image/png"), strings.HasSuffix(filename, ".png"): - return ".png", "image/png", true - case strings.HasPrefix(contentType, "image/webp"), strings.HasSuffix(filename, ".webp"): - return ".webp", "image/webp", true + case strings.HasPrefix(sniff, "image/jpeg"), + strings.HasPrefix(sniff, "image/png"), + strings.HasPrefix(sniff, "image/webp"): default: - return "", "", false + return nil, "", "", fmt.Errorf("unsupported type %s", sniff) + } + + img, format, err := image.Decode(bytes.NewReader(raw)) + if err != nil { + return nil, "", "", err + } + + var out bytes.Buffer + switch format { + case "jpeg": + if err := jpeg.Encode(&out, img, &jpeg.Options{Quality: 90}); err != nil { + return nil, "", "", err + } + return out.Bytes(), ".jpg", "image/jpeg", nil + case "png", "webp": + if err := png.Encode(&out, img); err != nil { + return nil, "", "", err + } + return out.Bytes(), ".png", "image/png", nil + default: + return nil, "", "", fmt.Errorf("unsupported format %s", format) } } diff --git a/internal/web/server.go b/internal/web/server.go index a6dd92d..c05666b 100644 --- a/internal/web/server.go +++ b/internal/web/server.go @@ -311,13 +311,13 @@ func (s *Server) handleSubmit(w http.ResponseWriter, r *http.Request) { return } if len(title) > 120 { - title = title[:120] + title = truncateRunes(title, 120) } if len(body) > 8000 { - body = body[:8000] + body = truncateRunes(body, 8000) } if len(city) > 80 { - city = city[:80] + city = truncateRunes(city, 80) } q := store.NewQuestion(s.db) q.AuthorID = u.ID @@ -449,7 +449,7 @@ func (s *Server) handleAnswer(w http.ResponseWriter, r *http.Request) { return } if len(body) > 12000 { - body = body[:12000] + body = truncateRunes(body, 12000) } ans := store.NewAnswer(s.db) ans.QuestionID = id @@ -518,6 +518,21 @@ func (s *Server) exec(w http.ResponseWriter, name string, data any) { } } +// truncateRunes shortens s to at most max runes without splitting a code point. +func truncateRunes(s string, max int) string { + if max <= 0 { + return "" + } + n := 0 + for byteIdx := range s { + if n == max { + return s[:byteIdx] + } + n++ + } + return s +} + func randomHex(n int) string { b := make([]byte, n) if _, err := rand.Read(b); err != nil { diff --git a/internal/web/server_test.go b/internal/web/server_test.go index d6d9fb7..530a8bd 100644 --- a/internal/web/server_test.go +++ b/internal/web/server_test.go @@ -4,6 +4,8 @@ import ( "bytes" "context" "database/sql" + "image" + "image/png" "mime/multipart" "net/http" "net/http/httptest" @@ -23,17 +25,14 @@ import ( func testDBURL() string { _ = godotenv.Load() - if u := strings.TrimSpace(os.Getenv("TEST_DATABASE_URL")); u != "" { - return u - } - return strings.TrimSpace(os.Getenv("DATABASE_URL")) + return strings.TrimSpace(os.Getenv("TEST_DATABASE_URL")) } func newTestServer(t *testing.T, cfg Config) (*Server, *sql.DB) { t.Helper() url := testDBURL() if url == "" { - t.Skip("set TEST_DATABASE_URL or DATABASE_URL for web tests") + t.Skip("set TEST_DATABASE_URL for web tests") } db, sessions, err := store.OpenPostgres(url, plumber.SchemaSQL) if err != nil { @@ -162,7 +161,7 @@ func TestRegisterLoginAsk(t *testing.T) { func TestSessionSurvivesServerRestart(t *testing.T) { url := testDBURL() if url == "" { - t.Skip("set TEST_DATABASE_URL or DATABASE_URL for web tests") + t.Skip("set TEST_DATABASE_URL for web tests") } db, sessions, err := store.OpenPostgres(url, plumber.SchemaSQL) if err != nil { @@ -511,7 +510,10 @@ func TestProfileAdminAnsweredListAndAvatarUpload(t *testing.T) { if err != nil { t.Fatal(err) } - _, _ = part.Write([]byte("fakepngbytes")) + img := image.NewRGBA(image.Rect(0, 0, 1, 1)) + if err := png.Encode(part, img); err != nil { + t.Fatal(err) + } _ = w.Close() req = httptest.NewRequest(http.MethodPost, "/profile", &buf) req.Header.Set("Content-Type", w.FormDataContentType()) diff --git a/todo.md b/todo.md index 5cac465..dbe675b 100644 --- a/todo.md +++ b/todo.md @@ -6,7 +6,7 @@ From the project review. Priority order within each section. - [x] **Persist sessions** — Sessions live in the app DB (`sessions` table) via `postgresstore`. Opaque cookie unchanged; unused `SESSION_SECRET` removed from config / `.env.example`. - [x] **Drop Dockerfile** — Deploying on DigitalOcean App Platform (buildpack from `go.mod`); no container image needed. -- [ ] **Rune-safe truncation** — `title[:120]`, `body[:8000]`, `city[:80]`, answer body, etc. can split multi-byte UTF-8. Truncate by runes (or safely). +- [x] **Rune-safe truncation** — `title[:120]`, `body[:8000]`, `city[:80]`, answer body, etc. can split multi-byte UTF-8. Truncate by runes (or safely). - [x] **Admin bootstrap** — `ADMIN_USERNAME` seeds the first admin on register only when no admin exists. Promote/demote via `/admin/users` (admins only); roles stay in `users.role`. ## Docs & ops