diff --git a/.env.example b/.env.example index 75cd168..a70b735 100644 --- a/.env.example +++ b/.env.example @@ -183,6 +183,18 @@ AMA_THANKYOU_COPY=Question submitted! It will appear once answered. # Strict-Transport-Security, Permissions-Policy) always ship. CSP_DISABLE=false +# ============================================================================= +# UPLOADS +# ============================================================================= + +# Set ORPHAN_SWEEP_DISABLED=true to skip the post-boot orphan-upload sweep. +# Default false: once per startup, MarkGo walks // and +# removes files no article references (by banner frontmatter field or body +# content — markdown image, raw HTML img/src, or plain link). Set to true +# if you manage files in the uploads tree independently of article +# references — otherwise unreferenced files will be cleaned on next boot. +ORPHAN_SWEEP_DISABLED=false + # ============================================================================= # LOGGING CONFIGURATION # ============================================================================= diff --git a/CHANGELOG.md b/CHANGELOG.md index 903a9b8..6a7af8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,44 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [3.18.1] - 2026-05-19 + +Wave-5 operator feedback patch. Four issues from log.1mb.dev. + +### Fixed + +- **CSRF cookie now stable across GETs (#101).** The middleware rotated + `_csrf` on every GET, even when a valid cookie was already present. + SPA navigation only swaps `
`, so the meta token rendered in + `` went stale relative to the rotated cookie and subsequent + XHRs (banner upload, etc.) 403'd. The middleware now reuses the + existing cookie when its value decodes as 32 raw bytes of hex; + rotation only on absence or malformed value. +- **Banner uploads no longer orphan on swap or remove (#103).** + `HandleEdit` unlinks the previous banner file when it matches + `/uploads//` shape and the operator swaps or clears + the field. External URLs, `/static/...` paths, and bare frontmatter + filenames are operator-managed and left alone. ENOENT silent; other + os.Remove failures warn-logged. +- **Autosave warning element contract pinned (#102).** The reported + "Draft saving unavailable" at edit-mode load is not reproducible + from the codebase: the template renders the warning element with + `hidden`, and `compose.js` only flips it visible after a + `localStorage.setItem` failure. Regression test added at the + template layer. + +### Added + +- **Startup orphan-upload sweep (#103).** One pass per boot walks + `//` and removes files no article references — + via banner field or body content (markdown image, raw HTML img/src, + or plain link). URL-decoded, traversal-rejected. Async post-bind, + 60s timeout bound. Disable with `ORPHAN_SWEEP_DISABLED=true` if + the operator manages files in the uploads tree independently of + article references. +- **`data-count` on `.tag-cloud-item` (#100).** Lets forkers size or + weight tags via CSS attribute selectors without JS. + ## [3.18.0] - 2026-05-19 Compose readability pass + "Save Draft" regression fix. diff --git a/internal/commands/serve/command.go b/internal/commands/serve/command.go index d5018e9..479750d 100644 --- a/internal/commands/serve/command.go +++ b/internal/commands/serve/command.go @@ -196,6 +196,8 @@ func setupServer(cfg *config.Config, logger *slog.Logger) (*gin.Engine, *service return nil, nil, nil, fmt.Errorf("article service: %w", err) } + go runOrphanSweep(cfg, articleService, logger) + emailService := services.NewEmailService(&cfg.Email, cfg.Blog.Title, logger) composeService := compose.NewService(cfg.ArticlesPath, cfg.Blog.Author) diff --git a/internal/commands/serve/sweep.go b/internal/commands/serve/sweep.go new file mode 100644 index 0000000..9dd058b --- /dev/null +++ b/internal/commands/serve/sweep.go @@ -0,0 +1,74 @@ +package serve + +import ( + "context" + "log/slog" + "time" + + "github.com/1mb-dev/markgo/internal/config" + "github.com/1mb-dev/markgo/internal/models" + "github.com/1mb-dev/markgo/internal/services/article" +) + +// sweepTimeout bounds the post-boot orphan sweep so a misconfigured upload +// directory cannot wedge the goroutine indefinitely. +const sweepTimeout = 60 * time.Second + +// articleLister is the slice of ArticleServiceInterface that the sweep +// needs. Declared locally to avoid importing the parent services package. +// +// GetPages is included alongside GetAllArticles because GetAllArticles +// applies the DedicatedRouteArticle predicate (v3.13.0) and excludes +// published type:page content from its result. Without GetPages in the +// reference set, Page banner uploads would appear unreferenced and be +// swept on every restart. +type articleLister interface { + GetAllArticles() []*models.Article + GetDraftArticles() []*models.Article + GetPages() []*models.Article +} + +// runOrphanSweep launches a single post-boot pass that removes upload +// files no article references. Honors ORPHAN_SWEEP_DISABLED. Logs a +// single slog.Info on completion with cleanup count and duration; errors +// during the walk are warn-logged inside the sweep itself. +// +// Recovers from any panic in the sweep path: best-effort cleanup must +// never bring down the server. A panic is logged at Error level so it +// surfaces in operator triage without losing the running process. +func runOrphanSweep(cfg *config.Config, articleSvc articleLister, logger *slog.Logger) { + defer func() { + if r := recover(); r != nil { + logger.Error("orphan sweep panicked", "panic", r) + } + }() + + if cfg.OrphanSweepDisabled { + logger.Info("orphan sweep disabled via ORPHAN_SWEEP_DISABLED") + return + } + if cfg.Upload.Path == "" { + return + } + + ctx, cancel := context.WithTimeout(context.Background(), sweepTimeout) + defer cancel() + + // GetAllArticles excludes dedicated-route content (type:page + about + // slug) via the v3.13.0 predicate. Bring Pages back in explicitly + // so Page banner uploads don't appear unreferenced. The /about + // article remains a known gap — operators using a banner there + // should rely on per-action unlink (Phase 3) and ORPHAN_SWEEP_DISABLED + // for the startup pass. + articles := articleSvc.GetAllArticles() + articles = append(articles, articleSvc.GetDraftArticles()...) + articles = append(articles, articleSvc.GetPages()...) + + start := time.Now() + cleaned, errs := article.OrphanSweep(ctx, articles, cfg.Upload.Path, logger) + logger.Info("orphan sweep complete", + "cleaned", cleaned, + "errors", len(errs), + "duration", time.Since(start), + ) +} diff --git a/internal/commands/serve/sweep_test.go b/internal/commands/serve/sweep_test.go new file mode 100644 index 0000000..cf0f850 --- /dev/null +++ b/internal/commands/serve/sweep_test.go @@ -0,0 +1,132 @@ +package serve + +import ( + "io" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/1mb-dev/markgo/internal/config" + "github.com/1mb-dev/markgo/internal/models" +) + +type stubArticleLister struct { + published []*models.Article + drafts []*models.Article + pages []*models.Article +} + +func (s *stubArticleLister) GetAllArticles() []*models.Article { return s.published } +func (s *stubArticleLister) GetDraftArticles() []*models.Article { return s.drafts } +func (s *stubArticleLister) GetPages() []*models.Article { return s.pages } + +func stageSweepFile(t *testing.T, root, slug, filename string) string { + t.Helper() + dir := filepath.Join(root, slug) + require.NoError(t, os.MkdirAll(dir, 0o755)) + p := filepath.Join(dir, filename) + require.NoError(t, os.WriteFile(p, []byte("x"), 0o600)) + return p +} + +func TestRunOrphanSweep_RemovesOrphansWhenEnabled(t *testing.T) { + uploadPath := t.TempDir() + orphan := stageSweepFile(t, uploadPath, "stale", "orphan.png") + + cfg := &config.Config{ + Upload: config.UploadConfig{Path: uploadPath}, + OrphanSweepDisabled: false, + } + lister := &stubArticleLister{} + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + runOrphanSweep(cfg, lister, logger) + + _, err := os.Stat(orphan) + assert.True(t, os.IsNotExist(err), "orphan must be removed when sweep enabled") +} + +func TestRunOrphanSweep_SkippedWhenDisabled(t *testing.T) { + uploadPath := t.TempDir() + orphan := stageSweepFile(t, uploadPath, "stale", "orphan.png") + + cfg := &config.Config{ + Upload: config.UploadConfig{Path: uploadPath}, + OrphanSweepDisabled: true, + } + lister := &stubArticleLister{} + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + runOrphanSweep(cfg, lister, logger) + + _, err := os.Stat(orphan) + assert.NoError(t, err, "orphan must survive when sweep disabled") +} + +func TestRunOrphanSweep_SkippedWhenUploadPathEmpty(t *testing.T) { + cfg := &config.Config{ + Upload: config.UploadConfig{Path: ""}, + OrphanSweepDisabled: false, + } + lister := &stubArticleLister{} + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + // Must not panic on empty upload path. + runOrphanSweep(cfg, lister, logger) +} + +// TestRunOrphanSweep_PreservesPageBanners is the regression test for the +// dedicated-route exclusion bug: GetAllArticles() filters out type:page +// articles via the DedicatedRouteArticle predicate (v3.13.0). Without +// GetPages() in the lister union, banner uploads referenced by published +// Page articles would appear unreferenced and be swept on next startup. +func TestRunOrphanSweep_PreservesPageBanners(t *testing.T) { + uploadPath := t.TempDir() + pageBanner := stageSweepFile(t, uploadPath, "info", "page-banner.png") + + cfg := &config.Config{ + Upload: config.UploadConfig{Path: uploadPath}, + OrphanSweepDisabled: false, + } + lister := &stubArticleLister{ + pages: []*models.Article{ + {Slug: "info", Banner: "/uploads/info/page-banner.png"}, + }, + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + runOrphanSweep(cfg, lister, logger) + + _, err := os.Stat(pageBanner) + assert.NoError(t, err, + "banner upload owned by a published Page article must survive the sweep") +} + +// panickyLister forces a panic when the sweep tries to enumerate articles, +// proving that runOrphanSweep's defer/recover keeps the goroutine alive. +type panickyLister struct{} + +func (panickyLister) GetAllArticles() []*models.Article { + panic("simulated lister failure") +} +func (panickyLister) GetDraftArticles() []*models.Article { return nil } +func (panickyLister) GetPages() []*models.Article { return nil } + +func TestRunOrphanSweep_RecoversFromPanic(t *testing.T) { + cfg := &config.Config{ + Upload: config.UploadConfig{Path: t.TempDir()}, + OrphanSweepDisabled: false, + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + // If recover() weren't in place, this call would propagate the panic + // up to the runtime — fatal in a goroutine. The fact that the test + // reaches the next line is the assertion. + assert.NotPanics(t, func() { + runOrphanSweep(cfg, panickyLister{}, logger) + }) +} diff --git a/internal/config/config.go b/internal/config/config.go index cd100dd..0c7ab96 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -47,6 +47,13 @@ type Config struct { Upload UploadConfig `json:"upload"` AMA AMAConfig `json:"ama"` Security SecurityConfig `json:"security"` + + // OrphanSweepDisabled skips the post-boot orphan-upload sweep when true. + // Default false: the sweep runs once per startup, removing files in + // // that no article (banner field or body content) + // references. Operators who manually drop assets into the uploads tree + // without referencing them from an article should set this to true. + OrphanSweepDisabled bool `json:"orphan_sweep_disabled"` } // SecurityConfig holds security-header configuration. Most security headers @@ -353,6 +360,8 @@ func Load() (*Config, error) { Security: SecurityConfig{ CSPDisable: getEnvBool("CSP_DISABLE", false), }, + + OrphanSweepDisabled: getEnvBool("ORPHAN_SWEEP_DISABLED", false), } // Validate the configuration diff --git a/internal/handlers/ama.go b/internal/handlers/ama.go index d264ce6..80007f6 100644 --- a/internal/handlers/ama.go +++ b/internal/handlers/ama.go @@ -133,7 +133,7 @@ func (h *AMAHandler) Answer(c *gin.Context) { input.Content = input.Content + "\n\n---\n\n" + form.Answer input.Draft = false - if err := h.composeService.UpdateArticle(slug, input); err != nil { + if _, err := h.composeService.UpdateArticle(slug, input); err != nil { h.handleError(c, err, "Failed to publish answer") return } diff --git a/internal/handlers/compose.go b/internal/handlers/compose.go index a6e13c6..4a160f5 100644 --- a/internal/handlers/compose.go +++ b/internal/handlers/compose.go @@ -225,7 +225,8 @@ func (h *ComposeHandler) HandleEdit(c *gin.Context) { return } - if err := h.composeService.UpdateArticle(slug, &input); err != nil { + prevBanner, err := h.composeService.UpdateArticle(slug, &input) + if err != nil { h.logger.Error("Failed to update post", "error", err, "slug", slug) data := h.buildBaseTemplateData("Edit - " + h.config.Blog.Title) data["template"] = templateCompose @@ -244,6 +245,7 @@ func (h *ComposeHandler) HandleEdit(c *gin.Context) { h.renderHTML(c, http.StatusInternalServerError, "base.html", data) return } + compose.UnlinkOwnedUpload(prevBanner, input.Banner, slug, h.config.Upload.Path, h.logger) reloadOK := true if err := h.articleService.ReloadArticles(); err != nil { @@ -477,7 +479,7 @@ func (h *ComposeHandler) PublishDraft(c *gin.Context) { } input.Draft = false - if err := h.composeService.UpdateArticle(slug, input); err != nil { + if _, err := h.composeService.UpdateArticle(slug, input); err != nil { h.logger.Error("Failed to publish draft", "error", err, "slug", slug) c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to publish draft"}) return diff --git a/internal/handlers/compose_test.go b/internal/handlers/compose_test.go index e345a34..bf463aa 100644 --- a/internal/handlers/compose_test.go +++ b/internal/handlers/compose_test.go @@ -1204,6 +1204,66 @@ func TestHandleEdit(t *testing.T) { assert.Equal(t, http.StatusBadRequest, w.Code) }) + t.Run("banner swap unlinks previous upload", func(t *testing.T) { + tmpDir := t.TempDir() + uploadPath := filepath.Join(tmpDir, "uploads") + cfg := &config.Config{ + Environment: "test", + BaseURL: "http://localhost:3000", + ArticlesPath: tmpDir, + Upload: config.UploadConfig{Path: uploadPath}, + Blog: config.BlogConfig{Title: "Test Blog", Author: "Test Author"}, + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + base := NewBaseHandler(cfg, logger, &MockTemplateService{}, &BuildInfo{Version: "test"}, &MockSEOService{}) + composeSvc := compose.NewService(tmpDir, cfg.Blog.Author) + handler := NewComposeHandler(base, composeSvc, &MockArticleService{}, &MockMarkdownRenderer{}) + + slug := "edit-banner-swap" + // Article on disk references uploads//old.png as its banner. + articleBody := fmt.Sprintf( + "---\nslug: %s\ntitle: Banner Owner\nbanner: /uploads/%s/old.png\ndraft: false\ndate: 2026-01-01T00:00:00Z\n---\n\nHello.\n", + slug, slug, + ) + articlePath := filepath.Join(tmpDir, "2026-01-01-"+slug+".md") + require.NoError(t, os.WriteFile(articlePath, []byte(articleBody), 0o644)) + + // Stage both files: previous (to be unlinked) and new (to be preserved). + slugUploadDir := filepath.Join(uploadPath, slug) + require.NoError(t, os.MkdirAll(slugUploadDir, 0o755)) + oldPath := filepath.Join(slugUploadDir, "old.png") + newPath := filepath.Join(slugUploadDir, "new.png") + require.NoError(t, os.WriteFile(oldPath, []byte("old"), 0o600)) + require.NoError(t, os.WriteFile(newPath, []byte("new"), 0o600)) + + router := gin.New() + router.Use(func(c *gin.Context) { + c.Set("csrf_secure", false) + c.Next() + }) + router.POST("/compose/edit/:slug", handler.HandleEdit) + + form := url.Values{ + "content": {"Updated body."}, + "title": {"Banner Owner"}, + "banner": {"/uploads/" + slug + "/new.png"}, + "banner_alt": {"new"}, + "_csrf": {"test-token"}, + } + req := httptest.NewRequest("POST", "/compose/edit/"+slug, strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + require.Equal(t, http.StatusSeeOther, w.Code, "edit must succeed") + + _, oldErr := os.Stat(oldPath) + assert.True(t, os.IsNotExist(oldErr), + "previous banner upload must be unlinked on swap; old.png still exists") + _, newErr := os.Stat(newPath) + assert.NoError(t, newErr, "new banner upload must be preserved") + }) + t.Run("edit toggles to draft → redirects to /admin/drafts", func(t *testing.T) { handler, tmpDir := createFormComposeHandler(t) writeDraftArticle(t, tmpDir, "edit-to-draft", false) @@ -1609,6 +1669,7 @@ func TestShowEdit(t *testing.T) { // handleError wraps this as ErrArticleNotFound → 404 assert.Equal(t, http.StatusNotFound, w.Code) }) + } // --------------------------------------------------------------------------- diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 20e3969..7b00078 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -381,8 +381,12 @@ const ( ) // CSRF implements double-submit cookie CSRF protection. -// On GET/HEAD: generates a token, sets it as an HttpOnly cookie, and stores it in gin context as "csrf_token". +// On GET/HEAD: reuses the existing _csrf cookie when present and well-formed (64-char hex); +// otherwise generates a new token and sets it as an HttpOnly cookie. Stored in gin context as "csrf_token". // On other methods (POST, PUT, DELETE): verifies the form field or X-CSRF-Token header matches the cookie value. +// The cookie is session-stable across reads — SPA navigations that only swap
+// must not invalidate the meta-rendered token in . Rotation happens only when +// the cookie is absent or malformed. // secureCookie controls the Secure flag — set false for localhost/HTTP development. func CSRF(secureCookie bool) gin.HandlerFunc { return func(c *gin.Context) { @@ -396,6 +400,13 @@ func CSRF(secureCookie bool) gin.HandlerFunc { return } } + // Reuse a well-formed existing cookie so SPA navigations don't rotate + // the token underneath stale meta tags rendered in a prior . + if existing, err := c.Cookie(csrfCookieName); err == nil && isValidCSRFToken(existing) { + c.Set("csrf_token", existing) + c.Next() + return + } token := generateCSRFToken() if token == "" { abortWithError(c, http.StatusInternalServerError, "Internal server error") diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go index da9e3c8..5e481a0 100644 --- a/internal/middleware/middleware_test.go +++ b/internal/middleware/middleware_test.go @@ -515,6 +515,132 @@ func TestCSRF(t *testing.T) { }) } +// TestCSRF_CookieStability covers the v3.18.1 fix: the middleware reuses a +// valid existing _csrf cookie on GET/HEAD instead of unconditionally +// regenerating it. SPA navigation only swaps
, so the meta tag in +// goes stale if the cookie rotates underneath it — the subsequent +// XHR's X-CSRF-Token header (from stale meta) then mismatches the new +// cookie and the POST 403s. Cookie must be session-stable. +func TestCSRF_CookieStability(t *testing.T) { + t.Run("GET preserves valid existing cookie", func(t *testing.T) { + router := setupTestRouter() + router.Use(CSRF(true)) + var contextToken string + router.GET("/page", func(c *gin.Context) { + if v, exists := c.Get("csrf_token"); exists { + contextToken, _ = v.(string) + } + c.String(200, "ok") + }) + + existing := generateCSRFToken() + require.NotEmpty(t, existing) + + req := httptest.NewRequest("GET", "/page", http.NoBody) + req.AddCookie(&http.Cookie{Name: "_csrf", Value: existing}) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + require.Equal(t, 200, w.Code) + assert.Equal(t, existing, contextToken, + "middleware must reuse the existing valid cookie, not generate a new token") + + // No rotating Set-Cookie should be emitted when the cookie is reused. + for _, c := range w.Result().Cookies() { + if c.Name == "_csrf" { + assert.Equal(t, existing, c.Value, + "if a Set-Cookie is emitted, it must echo the existing token, not a rotated one") + } + } + }) + + t.Run("GET generates when cookie absent", func(t *testing.T) { + router := setupTestRouter() + router.Use(CSRF(true)) + var contextToken string + router.GET("/page", func(c *gin.Context) { + if v, exists := c.Get("csrf_token"); exists { + contextToken, _ = v.(string) + } + c.String(200, "ok") + }) + + req := httptest.NewRequest("GET", "/page", http.NoBody) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + require.Equal(t, 200, w.Code) + require.NotEmpty(t, contextToken) + + var csrfCookie *http.Cookie + for _, c := range w.Result().Cookies() { + if c.Name == "_csrf" { + csrfCookie = c + } + } + require.NotNil(t, csrfCookie, "Set-Cookie must be emitted when no cookie was sent") + assert.Equal(t, contextToken, csrfCookie.Value) + }) + + t.Run("GET regenerates when cookie malformed", func(t *testing.T) { + router := setupTestRouter() + router.Use(CSRF(true)) + var contextToken string + router.GET("/page", func(c *gin.Context) { + if v, exists := c.Get("csrf_token"); exists { + contextToken, _ = v.(string) + } + c.String(200, "ok") + }) + + // Wrong length and non-hex characters — must be rejected, not trusted. + req := httptest.NewRequest("GET", "/page", http.NoBody) + req.AddCookie(&http.Cookie{Name: "_csrf", Value: "not-a-valid-hex-token"}) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + require.Equal(t, 200, w.Code) + require.NotEmpty(t, contextToken) + assert.NotEqual(t, "not-a-valid-hex-token", contextToken, + "middleware must not accept a malformed cookie as-is") + + var csrfCookie *http.Cookie + for _, c := range w.Result().Cookies() { + if c.Name == "_csrf" { + csrfCookie = c + } + } + require.NotNil(t, csrfCookie, "Set-Cookie must be emitted when malformed cookie is rejected") + assert.Equal(t, contextToken, csrfCookie.Value) + }) + + t.Run("POST validates against preserved cookie", func(t *testing.T) { + router := setupTestRouter() + router.Use(CSRF(true)) + router.GET("/api", func(c *gin.Context) { c.String(200, "ok") }) + router.POST("/api", func(c *gin.Context) { c.String(200, "posted") }) + + existing := generateCSRFToken() + require.NotEmpty(t, existing) + + // GET with the cookie — middleware should reuse it. + getReq := httptest.NewRequest("GET", "/api", http.NoBody) + getReq.AddCookie(&http.Cookie{Name: "_csrf", Value: existing}) + getW := httptest.NewRecorder() + router.ServeHTTP(getW, getReq) + require.Equal(t, 200, getW.Code) + + // POST with the original token — must succeed because cookie was preserved. + postReq := httptest.NewRequest("POST", "/api", strings.NewReader("")) + postReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") + postReq.Header.Set("X-CSRF-Token", existing) + postReq.AddCookie(&http.Cookie{Name: "_csrf", Value: existing}) + postW := httptest.NewRecorder() + router.ServeHTTP(postW, postReq) + assert.Equal(t, 200, postW.Code, "POST must validate against the preserved cookie") + }) +} + // TestSmartCacheHeaders tests the smart cache headers middleware func TestSmartCacheHeaders(t *testing.T) { router := setupTestRouter() diff --git a/internal/services/article/sweep.go b/internal/services/article/sweep.go new file mode 100644 index 0000000..cb95d13 --- /dev/null +++ b/internal/services/article/sweep.go @@ -0,0 +1,167 @@ +package article + +import ( + "context" + "errors" + "io/fs" + "log/slog" + "net/url" + "os" + "path/filepath" + "regexp" + "strings" + + "github.com/1mb-dev/markgo/internal/models" +) + +// uploadPathRE matches /uploads// shapes that may appear in +// article bodies (markdown images, raw HTML img/src, or plain links). +// Stops at whitespace, quotes, brackets, query-string, and fragment chars +// so that ![alt](url?v=2) yields filename "url" — matching what's on disk. +var uploadPathRE = regexp.MustCompile(`/uploads/([^/\s"'>))/", or "" when the banner +// is operator-managed (e.g. /static/...) and not eligible for tracking. +func bannerRelativePath(banner, slug string) string { + switch { + case strings.HasPrefix(banner, "/uploads/"): + return strings.TrimPrefix(banner, "/uploads/") + case strings.HasPrefix(banner, "uploads/"): + return strings.TrimPrefix(banner, "uploads/") + case strings.HasPrefix(banner, "/"): + return "" // /static/... and other server-absolute non-uploads + default: + return slug + "/" + banner + } +} + +// addBodyRefs scans content for /uploads// patterns and +// records each as a reference. +func addBodyRefs(refs map[string]struct{}, content, absBase string) { + for _, m := range uploadPathRE.FindAllStringSubmatch(content, -1) { + rel := m[1] + "/" + m[2] + addRefFromRel(refs, rel, absBase) + } +} + +// addRefFromRel resolves a "/" path against absBase, +// URL-decodes it, rejects traversal, and records the result. +func addRefFromRel(refs map[string]struct{}, rel, absBase string) { + if decoded, err := url.PathUnescape(rel); err == nil { + rel = decoded + } + if strings.Contains(rel, "..") { + return + } + abs, err := filepath.Abs(filepath.Join(absBase, rel)) + if err != nil { + return + } + if !strings.HasPrefix(abs, absBase+string(os.PathSeparator)) { + return + } + refs[abs] = struct{}{} +} + +// sweepUnreferenced walks absBase's slug subdirectories and removes any +// file whose absolute path is not in refs. +func sweepUnreferenced(ctx context.Context, refs map[string]struct{}, absBase string, logger *slog.Logger) (cleaned int, errs []error) { + entries, err := os.ReadDir(absBase) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return 0, nil + } + return 0, []error{err} + } + for _, slug := range entries { + if ctx.Err() != nil { + return cleaned, errs + } + if !slug.IsDir() { + continue + } + c, e := sweepSlugDir(ctx, refs, filepath.Join(absBase, slug.Name()), logger) + cleaned += c + errs = append(errs, e...) + } + return cleaned, errs +} + +// sweepSlugDir handles one slug directory. Split out from sweepUnreferenced +// to keep cyclomatic complexity bounded. +func sweepSlugDir(ctx context.Context, refs map[string]struct{}, slugDir string, logger *slog.Logger) (cleaned int, errs []error) { + files, err := os.ReadDir(slugDir) + if err != nil { + logger.Warn("orphan sweep: read slug dir failed", "dir", slugDir, "error", err) + return 0, []error{err} + } + for _, file := range files { + if ctx.Err() != nil { + return cleaned, errs + } + if file.IsDir() { + continue + } + path := filepath.Join(slugDir, file.Name()) + if _, referenced := refs[path]; referenced { + continue + } + if rmErr := os.Remove(path); rmErr != nil && !errors.Is(rmErr, fs.ErrNotExist) { + logger.Warn("orphan sweep: remove failed", "path", path, "error", rmErr) + errs = append(errs, rmErr) + continue + } + cleaned++ + } + return cleaned, errs +} diff --git a/internal/services/article/sweep_test.go b/internal/services/article/sweep_test.go new file mode 100644 index 0000000..ff43fc9 --- /dev/null +++ b/internal/services/article/sweep_test.go @@ -0,0 +1,218 @@ +package article + +import ( + "context" + "io" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/1mb-dev/markgo/internal/models" +) + +func stageOrphanFile(t *testing.T, uploadDir, slug, filename string) string { + t.Helper() + slugDir := filepath.Join(uploadDir, slug) + require.NoError(t, os.MkdirAll(slugDir, 0o755)) + path := filepath.Join(slugDir, filename) + require.NoError(t, os.WriteFile(path, []byte("payload"), 0o600)) + return path +} + +func quietSweepLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// TestOrphanSweep_DoesNotDeleteInlineReferencedFiles is the load-bearing +// safety test for Phase 4 of v3.18.1. An article with no banner field but +// an inline markdown image reference must protect that file from the +// sweep. Failing this test means the sweep is unsafe to ship. +func TestOrphanSweep_DoesNotDeleteInlineReferencedFiles(t *testing.T) { + uploadDir := t.TempDir() + slug := "post-with-inline" + inlinePath := stageOrphanFile(t, uploadDir, slug, "inline.png") + + article := &models.Article{ + Slug: slug, + Banner: "", + Content: "Some text.\n\n![alt](/uploads/" + slug + "/inline.png)\n\nMore text.", + } + + cleaned, errs := OrphanSweep( + context.Background(), + []*models.Article{article}, + uploadDir, + quietSweepLogger(), + ) + + assert.Empty(t, errs) + assert.Equal(t, 0, cleaned, "inline-referenced file must protect against sweep") + _, err := os.Stat(inlinePath) + assert.NoError(t, err, "inline.png must survive sweep") +} + +func TestOrphanSweep_DeletesUnreferencedOrphan(t *testing.T) { + uploadDir := t.TempDir() + orphanPath := stageOrphanFile(t, uploadDir, "stale-slug", "orphan.png") + + cleaned, errs := OrphanSweep( + context.Background(), + nil, // no articles reference anything + uploadDir, + quietSweepLogger(), + ) + + assert.Empty(t, errs) + assert.Equal(t, 1, cleaned) + _, err := os.Stat(orphanPath) + assert.True(t, os.IsNotExist(err), "orphan.png must be removed") +} + +func TestOrphanSweep_BannerFieldServerAbsoluteProtects(t *testing.T) { + uploadDir := t.TempDir() + slug := "post" + bannerPath := stageOrphanFile(t, uploadDir, slug, "banner.png") + + article := &models.Article{ + Slug: slug, + Banner: "/uploads/" + slug + "/banner.png", + } + + cleaned, _ := OrphanSweep(context.Background(), []*models.Article{article}, uploadDir, quietSweepLogger()) + assert.Equal(t, 0, cleaned) + _, err := os.Stat(bannerPath) + assert.NoError(t, err) +} + +func TestOrphanSweep_BannerFieldBareFilenameProtects(t *testing.T) { + uploadDir := t.TempDir() + slug := "post" + bannerPath := stageOrphanFile(t, uploadDir, slug, "bare.png") + + article := &models.Article{ + Slug: slug, + Banner: "bare.png", // bare filename — resolves to / + } + + cleaned, _ := OrphanSweep(context.Background(), []*models.Article{article}, uploadDir, quietSweepLogger()) + assert.Equal(t, 0, cleaned) + _, err := os.Stat(bannerPath) + assert.NoError(t, err) +} + +func TestOrphanSweep_ExternalURLDoesNotProtect(t *testing.T) { + uploadDir := t.TempDir() + slug := "post" + // File on disk whose name collides with an external URL's basename. The + // external URL must NOT preserve the local file. + collidingPath := stageOrphanFile(t, uploadDir, slug, "remote.png") + + article := &models.Article{ + Slug: slug, + Banner: "https://cdn.example.com/img/remote.png", + } + + cleaned, _ := OrphanSweep(context.Background(), []*models.Article{article}, uploadDir, quietSweepLogger()) + assert.Equal(t, 1, cleaned, "external banner URL must not preserve a locally orphaned file") + _, err := os.Stat(collidingPath) + assert.True(t, os.IsNotExist(err)) +} + +func TestOrphanSweep_HTMLImgSrcProtects(t *testing.T) { + uploadDir := t.TempDir() + slug := "post" + imgPath := stageOrphanFile(t, uploadDir, slug, "html-img.png") + + article := &models.Article{ + Slug: slug, + Content: `Body with raw HTML: x`, + } + + cleaned, _ := OrphanSweep(context.Background(), []*models.Article{article}, uploadDir, quietSweepLogger()) + assert.Equal(t, 0, cleaned, "raw HTML img src must protect referenced file") + _, err := os.Stat(imgPath) + assert.NoError(t, err) +} + +func TestOrphanSweep_QueryStringInBodyStillProtects(t *testing.T) { + uploadDir := t.TempDir() + slug := "post" + imgPath := stageOrphanFile(t, uploadDir, slug, "queried.png") + + article := &models.Article{ + Slug: slug, + Content: `Look: ![x](/uploads/` + slug + `/queried.png?v=2)`, + } + + cleaned, _ := OrphanSweep(context.Background(), []*models.Article{article}, uploadDir, quietSweepLogger()) + assert.Equal(t, 0, cleaned, "query-string suffix must not break the protection") + _, err := os.Stat(imgPath) + assert.NoError(t, err) +} + +func TestOrphanSweep_CrossSlugReferenceProtects(t *testing.T) { + uploadDir := t.TempDir() + // File lives under slug-a, but is referenced from an article whose own + // slug is slug-b. Body-content scan must still recognize this. + imgPath := stageOrphanFile(t, uploadDir, "slug-a", "shared.png") + + article := &models.Article{ + Slug: "slug-b", + Content: "![x](/uploads/slug-a/shared.png)", + } + + cleaned, _ := OrphanSweep(context.Background(), []*models.Article{article}, uploadDir, quietSweepLogger()) + assert.Equal(t, 0, cleaned, "cross-slug body reference must protect the file") + _, err := os.Stat(imgPath) + assert.NoError(t, err) +} + +func TestOrphanSweep_RespectsContextCancellation(t *testing.T) { + uploadDir := t.TempDir() + stageOrphanFile(t, uploadDir, "slug-1", "a.png") + stageOrphanFile(t, uploadDir, "slug-2", "b.png") + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately + + cleaned, _ := OrphanSweep(ctx, nil, uploadDir, quietSweepLogger()) + // Cancelled before any work — must not panic; may have cleaned 0. + assert.LessOrEqual(t, cleaned, 2) +} + +func TestOrphanSweep_NonexistentUploadDir(t *testing.T) { + cleaned, errs := OrphanSweep( + context.Background(), + nil, + filepath.Join(t.TempDir(), "does-not-exist"), + quietSweepLogger(), + ) + assert.Empty(t, errs, "missing upload dir is not an error — operator may not have uploaded yet") + assert.Equal(t, 0, cleaned) +} + +func TestOrphanSweep_MultipleOrphansAcrossSlugs(t *testing.T) { + uploadDir := t.TempDir() + orphan1 := stageOrphanFile(t, uploadDir, "slug-a", "orphan-1.png") + orphan2 := stageOrphanFile(t, uploadDir, "slug-a", "orphan-2.png") + kept := stageOrphanFile(t, uploadDir, "slug-a", "kept.png") + orphan3 := stageOrphanFile(t, uploadDir, "slug-b", "orphan-3.png") + + article := &models.Article{ + Slug: "slug-a", + Banner: "/uploads/slug-a/kept.png", + } + + cleaned, _ := OrphanSweep(context.Background(), []*models.Article{article}, uploadDir, quietSweepLogger()) + assert.Equal(t, 3, cleaned) + for _, p := range []string{orphan1, orphan2, orphan3} { + _, err := os.Stat(p) + assert.True(t, os.IsNotExist(err), "expected %s to be removed", p) + } + _, err := os.Stat(kept) + assert.NoError(t, err, "banner-referenced file must survive") +} diff --git a/internal/services/compose/service.go b/internal/services/compose/service.go index 1d06fcc..6a772b1 100644 --- a/internal/services/compose/service.go +++ b/internal/services/compose/service.go @@ -229,22 +229,26 @@ func (s *Service) LoadArticle(slug string) (*Input, error) { // UpdateArticle overwrites an existing article file with updated content. // Preserves fields not exposed in the compose form (slug, date, author). -func (s *Service) UpdateArticle(slug string, input *Input) error { +// Returns the previous frontmatter banner value (empty when unset) so +// callers can clean up orphan uploads on swap or remove. +func (s *Service) UpdateArticle(slug string, input *Input) (prevBanner string, err error) { filePath, existingContent, err := s.findFileBySlug(slug) if err != nil { - return err + return "", err } parts := strings.SplitN(string(existingContent), "---", 3) if len(parts) < 3 { - return fmt.Errorf("invalid article format: missing frontmatter") + return "", fmt.Errorf("invalid article format: missing frontmatter") } var fm map[string]any if unmarshalErr := yaml.Unmarshal([]byte(parts[1]), &fm); unmarshalErr != nil { - return fmt.Errorf("failed to parse frontmatter: %w", unmarshalErr) + return "", fmt.Errorf("failed to parse frontmatter: %w", unmarshalErr) } + prevBanner = stringFieldOr(fm, "banner", "") + // Update editable fields, preserve everything else (slug, date, author) if input.Title != "" { fm["title"] = input.Title @@ -306,7 +310,7 @@ func (s *Service) UpdateArticle(slug string, input *Input) error { yamlBytes, err := yaml.Marshal(fm) if err != nil { - return fmt.Errorf("failed to marshal frontmatter: %w", err) + return "", fmt.Errorf("failed to marshal frontmatter: %w", err) } fileContent := fmt.Sprintf("---\n%s---\n\n%s\n", string(yamlBytes), strings.TrimSpace(input.Content)) @@ -314,25 +318,25 @@ func (s *Service) UpdateArticle(slug string, input *Input) error { // Atomic write: temp file + rename prevents data loss on crash tmpFile, err := os.CreateTemp(filepath.Dir(filePath), ".markgo-*.tmp") if err != nil { - return fmt.Errorf("failed to create temp file: %w", err) + return "", fmt.Errorf("failed to create temp file: %w", err) } tmpPath := tmpFile.Name() if _, err := tmpFile.WriteString(fileContent); err != nil { tmpFile.Close() //nolint:gosec // best-effort cleanup os.Remove(tmpPath) //nolint:gosec // best-effort cleanup - return fmt.Errorf("failed to write temp file: %w", err) + return "", fmt.Errorf("failed to write temp file: %w", err) } if err := tmpFile.Close(); err != nil { os.Remove(tmpPath) //nolint:gosec // best-effort cleanup - return fmt.Errorf("failed to close temp file: %w", err) + return "", fmt.Errorf("failed to close temp file: %w", err) } if err := os.Rename(tmpPath, filePath); err != nil { os.Remove(tmpPath) //nolint:gosec // best-effort cleanup - return fmt.Errorf("failed to rename temp file: %w", err) + return "", fmt.Errorf("failed to rename temp file: %w", err) } - return nil + return prevBanner, nil } // DeletePost removes an article file by slug. @@ -464,6 +468,16 @@ func setIfNonEmpty(m map[string]any, key, val string) { } } +// stringFieldOr returns m[key] as a string, or fallback when the key is +// missing or not a string. Lifted out of UpdateArticle to keep its +// cyclomatic budget intact while still reading the previous banner value. +func stringFieldOr(m map[string]any, key, fallback string) string { + if v, ok := m[key].(string); ok { + return v + } + return fallback +} + func generateSlug(title string) string { slug := strings.ToLower(title) slug = strings.ReplaceAll(slug, " ", "-") diff --git a/internal/services/compose/service_test.go b/internal/services/compose/service_test.go index a78aff6..bf85a20 100644 --- a/internal/services/compose/service_test.go +++ b/internal/services/compose/service_test.go @@ -165,7 +165,7 @@ func TestUpdateArticle(t *testing.T) { require.NoError(t, err) // Update it with description and categories - err = svc.UpdateArticle(slug, &Input{ + _, err = svc.UpdateArticle(slug, &Input{ Title: "Updated Title", Description: "Updated description for SEO", Content: "Updated content with **markdown**.", @@ -200,7 +200,7 @@ func TestUpdateArticle_PreservesMetadata(t *testing.T) { require.NoError(t, err) // Update only content - err = svc.UpdateArticle(slug, &Input{ + _, err = svc.UpdateArticle(slug, &Input{ Title: "Metadata Test", Content: "New content.", }) @@ -431,7 +431,7 @@ func TestUpdateArticle_PreservesAMAFields(t *testing.T) { require.NoError(t, err) // Update content (simulate answering) — don't set Asker/Type in input - err = svc.UpdateArticle(slug, &Input{ + _, err = svc.UpdateArticle(slug, &Input{ Content: "What is your favorite language?\n\n---\n\nGo, for its simplicity.", Draft: false, }) @@ -589,7 +589,7 @@ func TestUpdateArticle_BannerAltDroppedWhenBannerRemoved(t *testing.T) { // User clicks "Remove banner" but the form still posts the old alt text // (the bug this test guards against). Server must drop banner_alt too. - err = svc.UpdateArticle(slug, &Input{ + _, err = svc.UpdateArticle(slug, &Input{ Title: "Has Banner", Content: "Body.", Banner: "", @@ -630,7 +630,7 @@ func TestUpdateArticle_BannerPreservedOnEdit(t *testing.T) { // Simulate an edit that doesn't touch the banner: same Banner/BannerAlt // submitted (the hidden field round-trips the existing value). - err = svc.UpdateArticle("editorial", &Input{ + _, err = svc.UpdateArticle("editorial", &Input{ Title: "Editorial Piece (edited)", Content: "Body with edits.", Banner: loaded.Banner, diff --git a/internal/services/compose/unlink.go b/internal/services/compose/unlink.go new file mode 100644 index 0000000..84bf761 --- /dev/null +++ b/internal/services/compose/unlink.go @@ -0,0 +1,54 @@ +package compose + +import ( + "errors" + "io/fs" + "log/slog" + "os" + "path/filepath" + "strings" +) + +// UnlinkOwnedUpload removes an upload referenced by the server-absolute +// path shape /uploads// when the banner has changed. +// Side-effect cleanup intended for callers who just persisted a new banner +// value to disk. +// +// No-op when: +// - prev is empty (no previous upload to clean). +// - prev equals next (banner unchanged). +// - prev is not in the /uploads// shape (external URL, /static/..., +// or operator-managed bare filename — those are not ours to remove). +// - filename contains a path separator (defense against malformed input +// that would land outside the slug directory). +// +// Path-containment is enforced via ContainSlugPath; a containment failure +// is logged and skipped. os.ErrNotExist is silent (the file was already +// gone). Any other os.Remove error is logged at warn level. +// +// Cleanup is best-effort and must never fail the caller; this function +// has no return value by design. +func UnlinkOwnedUpload(prev, next, slug, uploadPath string, logger *slog.Logger) { + if prev == "" || prev == next { + return + } + expectedPrefix := "/uploads/" + slug + "/" + if !strings.HasPrefix(prev, expectedPrefix) { + return + } + filename := strings.TrimPrefix(prev, expectedPrefix) + if filename == "" || strings.ContainsRune(filename, '/') { + return + } + slugDir, err := ContainSlugPath(uploadPath, slug) + if err != nil { + logger.Warn("unlink owned upload: containment failed", + "slug", slug, "prev_banner", prev, "error", err) + return + } + target := filepath.Join(slugDir, filename) + if err := os.Remove(target); err != nil && !errors.Is(err, fs.ErrNotExist) { + logger.Warn("unlink owned upload: remove failed", + "slug", slug, "path", target, "error", err) + } +} diff --git a/internal/services/compose/unlink_test.go b/internal/services/compose/unlink_test.go new file mode 100644 index 0000000..fed2bd3 --- /dev/null +++ b/internal/services/compose/unlink_test.go @@ -0,0 +1,182 @@ +package compose + +import ( + "io" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fileExists is a small helper for unlink assertions. +func fileExists(t *testing.T, path string) bool { + t.Helper() + _, err := os.Stat(path) + return err == nil +} + +// stageUpload creates an upload file at // and +// returns its absolute path. +func stageUpload(t *testing.T, uploadPath, slug, filename string) string { + t.Helper() + slugDir := filepath.Join(uploadPath, slug) + require.NoError(t, os.MkdirAll(slugDir, 0o755)) + path := filepath.Join(slugDir, filename) + require.NoError(t, os.WriteFile(path, []byte("payload"), 0o600)) + return path +} + +func quietLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +func TestUnlinkOwnedUpload_Swap(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-a" + prevPath := stageUpload(t, uploadPath, slug, "old.png") + nextPath := stageUpload(t, uploadPath, slug, "new.png") + + UnlinkOwnedUpload( + "/uploads/"+slug+"/old.png", + "/uploads/"+slug+"/new.png", + slug, uploadPath, quietLogger(), + ) + + assert.False(t, fileExists(t, prevPath), "previous banner must be unlinked on swap") + assert.True(t, fileExists(t, nextPath), "new banner must be preserved") +} + +func TestUnlinkOwnedUpload_Remove(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-b" + prevPath := stageUpload(t, uploadPath, slug, "removed.png") + + UnlinkOwnedUpload( + "/uploads/"+slug+"/removed.png", + "", // banner cleared + slug, uploadPath, quietLogger(), + ) + + assert.False(t, fileExists(t, prevPath), "previous banner must be unlinked on remove") +} + +func TestUnlinkOwnedUpload_NoChange(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-c" + keepPath := stageUpload(t, uploadPath, slug, "kept.png") + + UnlinkOwnedUpload( + "/uploads/"+slug+"/kept.png", + "/uploads/"+slug+"/kept.png", + slug, uploadPath, quietLogger(), + ) + + assert.True(t, fileExists(t, keepPath), "unchanged banner must not be touched") +} + +func TestUnlinkOwnedUpload_ExternalURLNotTouched(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-d" + // Stage a file at a path that COULD collide with the external URL's + // basename, to prove the helper doesn't accidentally derive a local path. + collidingPath := stageUpload(t, uploadPath, slug, "remote.png") + + UnlinkOwnedUpload( + "https://cdn.example.com/img/remote.png", + "/uploads/"+slug+"/new.png", + slug, uploadPath, quietLogger(), + ) + + assert.True(t, fileExists(t, collidingPath), + "external URLs are operator-managed and must never trigger an unlink") +} + +func TestUnlinkOwnedUpload_StaticPathNotTouched(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-e" + // /static/... paths are STATIC_PATH-overlay territory, not slug uploads. + UnlinkOwnedUpload( + "/static/img/site-banner.png", + "/uploads/"+slug+"/new.png", + slug, uploadPath, quietLogger(), + ) + // No file to verify — the assertion is that the function returns without + // touching anything outside uploadPath. Coverage proves the branch. +} + +func TestUnlinkOwnedUpload_BareFilenameNotTouched(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-f" + // Bare filename in frontmatter (operator-edited markdown form) resolves + // to /uploads//bare.png via models.Article.BannerSrc, but the + // frontmatter literal lacks the leading-slash shape. Operator owns these. + keepPath := stageUpload(t, uploadPath, slug, "bare.png") + + UnlinkOwnedUpload( + "bare.png", + "/uploads/"+slug+"/new.png", + slug, uploadPath, quietLogger(), + ) + + assert.True(t, fileExists(t, keepPath), + "bare frontmatter filenames are operator-managed and must not be unlinked") +} + +func TestUnlinkOwnedUpload_PathTraversalRejected(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-g" + // File outside slug dir — must survive an attempted traversal. + outsideDir := filepath.Join(uploadPath, "..", "victim") + require.NoError(t, os.MkdirAll(outsideDir, 0o755)) + outsidePath := filepath.Join(outsideDir, "evil.png") + require.NoError(t, os.WriteFile(outsidePath, []byte("payload"), 0o600)) + + UnlinkOwnedUpload( + "/uploads/"+slug+"/../victim/evil.png", + "", + slug, uploadPath, quietLogger(), + ) + + assert.True(t, fileExists(t, outsidePath), + "path traversal in prev banner must not unlink files outside slug dir") +} + +func TestUnlinkOwnedUpload_ENOENTSilent(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-h" + // No file staged — prev points at a nonexistent file. + + // Capture logger output to verify no warn-level log fires for ENOENT. + var buf bytesBuffer + logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn})) + + UnlinkOwnedUpload( + "/uploads/"+slug+"/never-existed.png", + "", + slug, uploadPath, logger, + ) + + assert.Empty(t, buf.String(), + "ENOENT on os.Remove must be silent — no warn log expected") +} + +func TestUnlinkOwnedUpload_EmptyPrevNoOp(t *testing.T) { + uploadPath := t.TempDir() + slug := "post-i" + // Empty prev — first-time banner upload, nothing to clean. + UnlinkOwnedUpload("", "/uploads/"+slug+"/first.png", slug, uploadPath, quietLogger()) + // Reaching this line without panic is the assertion. +} + +// bytesBuffer is a tiny io.Writer used to capture slog output in tests. +type bytesBuffer struct{ data []byte } + +func (b *bytesBuffer) Write(p []byte) (int, error) { + b.data = append(b.data, p...) + return len(p), nil +} + +func (b *bytesBuffer) String() string { return string(b.data) } diff --git a/internal/services/template_test.go b/internal/services/template_test.go index 3ba9539..d924c03 100644 --- a/internal/services/template_test.go +++ b/internal/services/template_test.go @@ -4,6 +4,7 @@ import ( "html/template" "os" "path/filepath" + "regexp" "strings" "testing" "testing/fstest" @@ -14,6 +15,7 @@ import ( "github.com/1mb-dev/markgo/internal/config" "github.com/1mb-dev/markgo/internal/models" + "github.com/1mb-dev/markgo/web" ) func TestNewTemplateService(t *testing.T) { @@ -1026,3 +1028,19 @@ func TestBrandLogo_RendersThroughTemplateService(t *testing.T) { assert.Contains(t, out, `id="operator-marker"`, "rendered HTML must contain operator SVG") assert.Contains(t, out, `class="brand-logo"`, "class must be injected end-to-end") } + +// TestComposeTemplate_SaveWarningHiddenByDefault pins the #102 server-side +// contract: the autosave warning element must always be rendered with the +// `hidden` attribute. The only path to visible-on-screen is compose.js +// flipping it after a localStorage.setItem failure. Without `hidden`, CSS +// does not suppress display and the operator sees the warning at initial +// load. If this test breaks, a future template change has removed the +// guard. +func TestComposeTemplate_SaveWarningHiddenByDefault(t *testing.T) { + body, err := web.Assets.ReadFile("templates/compose.html") + require.NoError(t, err, "embedded compose.html must be readable") + + pattern := regexp.MustCompile(`]*\bid="compose-save-warning"[^>]*\bhidden\b[^>]*>`) + assert.Regexp(t, pattern, string(body), + "compose-save-warning must carry the hidden attribute at initial render") +} diff --git a/web/templates/tags.html b/web/templates/tags.html index fe36f0a..9bf8927 100644 --- a/web/templates/tags.html +++ b/web/templates/tags.html @@ -9,7 +9,7 @@

Tags

{{ if .tags }}
{{ range .tags }} - + {{ .Tag }} {{ .Count }}