From 099e46ac6a2141f17757b3625a2e1e1a4125c04c Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 17:48:55 +0545 Subject: [PATCH 1/9] fix(middleware): preserve valid CSRF cookie across GETs SPA navigations swap
only; rotating the cookie on GET left the meta token stale, mismatching subsequent POSTs (banner upload 403). Closes #101. --- internal/middleware/middleware.go | 13 ++- internal/middleware/middleware_test.go | 126 +++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 1 deletion(-) 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() From 6223db822151e79d11c1bb0502ea1549e10b7389 Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 17:56:35 +0545 Subject: [PATCH 2/9] test(services): pin compose save-warning hidden contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server template renders #compose-save-warning with `hidden` unconditionally; only compose.js's localStorage write-failure handler flips it visible. Reporter's #102 symptom is not reproducible from the code — autosave is purely client-side, no network path links it to #101. Refs #102. --- internal/services/template_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) 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") +} From 9a36b7a55bb681f490299992b3b12bd5efb7a079 Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 18:07:37 +0545 Subject: [PATCH 3/9] fix(handlers): unlink previous banner upload on swap or remove UpdateArticle now returns the previous banner value alongside its error. HandleEdit passes that to UnlinkOwnedUpload, which removes the file only when it matches /uploads// shape and lives inside the slug's upload dir. External URLs, /static/... paths, and bare filenames are operator-managed and left alone. Closes #103. --- internal/handlers/ama.go | 2 +- internal/handlers/compose.go | 6 +- internal/handlers/compose_test.go | 61 ++++++++ internal/services/compose/service.go | 34 ++-- internal/services/compose/service_test.go | 10 +- internal/services/compose/unlink.go | 54 +++++++ internal/services/compose/unlink_test.go | 182 ++++++++++++++++++++++ 7 files changed, 331 insertions(+), 18 deletions(-) create mode 100644 internal/services/compose/unlink.go create mode 100644 internal/services/compose/unlink_test.go 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/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) } From ea7977619da9ad98e301280cfea26ff246e08b0f Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 18:17:52 +0545 Subject: [PATCH 4/9] feat(services): startup orphan sweep with reference tracking Walks // once per boot and removes files no article references via banner field or body content (markdown image, raw HTML img/src, plain link). URL-decoded, traversal-rejected. Bounded by 60s timeout, runs async post-bind. Disable with ORPHAN_SWEEP_DISABLED=true. Refs #103. --- internal/commands/serve/command.go | 2 + internal/commands/serve/sweep.go | 49 ++++++ internal/commands/serve/sweep_test.go | 78 +++++++++ internal/config/config.go | 9 + internal/services/article/sweep.go | 167 ++++++++++++++++++ internal/services/article/sweep_test.go | 218 ++++++++++++++++++++++++ 6 files changed, 523 insertions(+) create mode 100644 internal/commands/serve/sweep.go create mode 100644 internal/commands/serve/sweep_test.go create mode 100644 internal/services/article/sweep.go create mode 100644 internal/services/article/sweep_test.go 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..d0ddd88 --- /dev/null +++ b/internal/commands/serve/sweep.go @@ -0,0 +1,49 @@ +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. +type articleLister interface { + GetAllArticles() []*models.Article + GetDraftArticles() []*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. +func runOrphanSweep(cfg *config.Config, articleSvc articleLister, logger *slog.Logger) { + 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() + + articles := append(articleSvc.GetAllArticles(), articleSvc.GetDraftArticles()...) + + 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..4ccaffc --- /dev/null +++ b/internal/commands/serve/sweep_test.go @@ -0,0 +1,78 @@ +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 +} + +func (s *stubArticleLister) GetAllArticles() []*models.Article { return s.published } +func (s *stubArticleLister) GetDraftArticles() []*models.Article { return s.drafts } + +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) +} 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/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") +} From b9cf0ac2a112f2cba0d4f4f3f1c9c5f6e6be8d0f Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 18:19:36 +0545 Subject: [PATCH 5/9] feat(templates): emit data-count on tag-cloud items Lets forkers size or weight tags via CSS attribute selectors without JS. Operator request, no semantics change for existing markup. Closes #100. --- web/templates/tags.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 }} From 70a0a18b043f0bc0b7f0f3a7bfbfdb0e481d1bd0 Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 18:22:08 +0545 Subject: [PATCH 6/9] docs(changelog): v3.18.1 release notes Wave-5 operator feedback patch: #100, #101, #102, #103. --- CHANGELOG.md | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) 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. From cbc0637d6b429cec4b39c34b2e3916e4e7523473 Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 18:26:26 +0545 Subject: [PATCH 7/9] docs(env): document ORPHAN_SWEEP_DISABLED in .env.example --- .env.example | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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 # ============================================================================= From 6b095253380d3fefc4ed5e06c95f2ec28d849a61 Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 18:27:24 +0545 Subject: [PATCH 8/9] fix(serve): recover from panics in orphan-sweep goroutine Best-effort cleanup must not crash the server. An unrecovered panic in runOrphanSweep would propagate to the runtime and bring down the process. Logged at Error level so it still surfaces in triage. --- internal/commands/serve/sweep.go | 10 ++++++++++ internal/commands/serve/sweep_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/internal/commands/serve/sweep.go b/internal/commands/serve/sweep.go index d0ddd88..993e88a 100644 --- a/internal/commands/serve/sweep.go +++ b/internal/commands/serve/sweep.go @@ -25,7 +25,17 @@ type articleLister interface { // 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 diff --git a/internal/commands/serve/sweep_test.go b/internal/commands/serve/sweep_test.go index 4ccaffc..037f27d 100644 --- a/internal/commands/serve/sweep_test.go +++ b/internal/commands/serve/sweep_test.go @@ -76,3 +76,27 @@ func TestRunOrphanSweep_SkippedWhenUploadPathEmpty(t *testing.T) { // Must not panic on empty upload path. runOrphanSweep(cfg, lister, logger) } + +// 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 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) + }) +} From 0ed1c33ca0db0d78c309fd3adece092fad00fc94 Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Tue, 19 May 2026 18:47:44 +0545 Subject: [PATCH 9/9] fix(serve): include Pages in orphan-sweep reference set GetAllArticles() applies the v3.13.0 DedicatedRouteArticle predicate and omits type:page content. Without GetPages() in the union, Page banner uploads appeared unreferenced and were swept on every restart. --- internal/commands/serve/sweep.go | 17 ++++++++++++++- internal/commands/serve/sweep_test.go | 30 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/internal/commands/serve/sweep.go b/internal/commands/serve/sweep.go index 993e88a..9dd058b 100644 --- a/internal/commands/serve/sweep.go +++ b/internal/commands/serve/sweep.go @@ -16,9 +16,16 @@ 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 @@ -47,7 +54,15 @@ func runOrphanSweep(cfg *config.Config, articleSvc articleLister, logger *slog.L ctx, cancel := context.WithTimeout(context.Background(), sweepTimeout) defer cancel() - articles := append(articleSvc.GetAllArticles(), articleSvc.GetDraftArticles()...) + // 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) diff --git a/internal/commands/serve/sweep_test.go b/internal/commands/serve/sweep_test.go index 037f27d..cf0f850 100644 --- a/internal/commands/serve/sweep_test.go +++ b/internal/commands/serve/sweep_test.go @@ -17,10 +17,12 @@ import ( 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() @@ -77,6 +79,33 @@ func TestRunOrphanSweep_SkippedWhenUploadPathEmpty(t *testing.T) { 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{} @@ -85,6 +114,7 @@ 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{