From b6d854538aef0f9d703f0503e6a0afe88d5d0e23 Mon Sep 17 00:00:00 2001 From: ssavutu Date: Fri, 31 Jul 2026 23:38:57 -0400 Subject: [PATCH] fix(media): store uploads world-readable so the media server can serve them An uploaded image landed on disk at the right path but every request for it got 403 from Nginx. storeUpload writes to a temp file and renames it into place. os.CreateTemp hardcodes mode 0600, and os.Rename moves the temp file's inode onto the destination -- so the 0644 that reserveAndRename uses to claim the name is discarded along with the empty file it created. Every uploaded asset ended up readable only by the CMS's own uid, while the Nginx worker serving /wp-content/ runs as another user and got EACCES. Chmod the temp file before the rename, and best-effort widen a freshly created YYYY/MM directory, whose mode is masked by the process umask and can produce an identical 403 on the first upload of a month. This predates the editor work -- it just could not surface while uploading was admin-only and no uploaded image had been fetched back through Nginx. Co-Authored-By: Claude Opus 5 --- server/internal/handlers/media.go | 33 +++++++++++++++++++ server/internal/handlers/media_test.go | 44 ++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/server/internal/handlers/media.go b/server/internal/handlers/media.go index bfd3124..20a0932 100644 --- a/server/internal/handlers/media.go +++ b/server/internal/handlers/media.go @@ -213,6 +213,13 @@ func storeImage(ctx context.Context, conn *sql.DB, src io.ReadSeeker, filename s slog.Error("media upload: create directory", "dir", absDir, "error", err) return models.MediaUploadResponse{}, fmt.Errorf("%w: create directory: %v", errStoreFailed, err) } + // MkdirAll's mode is masked by the process umask, so the first upload of a + // new month can leave YYYY/ or YYYY/MM without the world-execute bit the + // Nginx worker needs to traverse into it -- a 403 indistinguishable from the + // file-mode one. Best effort: a directory the ETL's rsync already created is + // owned by another uid and cannot be chmod'ed by us, which is fine because + // that one is already correct. + ensureTraversable(filepath.Dir(absDir), absDir) name, written, err := storeUpload(absDir, sanitizeBaseName(filename), ext, src) if err != nil { @@ -466,6 +473,22 @@ func sanitizeBaseName(filename string) string { return base } +// ensureTraversable best-effort widens directory permissions to 0775 so the +// media server can descend into directories this process created. Failures are +// ignored on purpose: the only way chmod fails here is that someone else owns +// the directory, which means it predates us and already has working modes. +func ensureTraversable(dirs ...string) { + for _, dir := range dirs { + info, err := os.Stat(dir) + if err != nil || info.Mode().Perm() == 0o775 { + continue + } + if err := os.Chmod(dir, 0o775); err != nil { + slog.Debug("media upload: could not widen directory mode", "dir", dir, "error", err) + } + } +} + // storeUpload writes src to a uniquely-named file in dir, never overwriting an // existing asset (important: the legacy corpus lives here too). It writes to a // temp file first and atomically renames into place so partially-written files @@ -484,6 +507,16 @@ func storeUpload(dir, base, ext string, src io.Reader) (name string, size int64, } }() + // os.CreateTemp always creates with mode 0600, and os.Rename moves the temp + // file's *inode* onto the destination -- so the 0644 that reserveAndRename + // uses to claim the name is discarded along with the file it created. Without + // this the stored asset is readable only by the CMS's own uid, and the Nginx + // worker serving /wp-content/ answers 403 for every uploaded image. Chmod is + // not subject to the umask, which is what we want here. + if err = tmp.Chmod(0o644); err != nil { + return "", 0, err + } + if size, err = io.Copy(tmp, src); err != nil { return "", 0, err } diff --git a/server/internal/handlers/media_test.go b/server/internal/handlers/media_test.go index ac99ab5..4e9c6c7 100644 --- a/server/internal/handlers/media_test.go +++ b/server/internal/handlers/media_test.go @@ -397,3 +397,47 @@ func TestGetMediaIndexStatus_ReportsIdle(t *testing.T) { t.Fatal("expected the shared job to be idle") } } + +// storeUpload renames a temp file into place, and os.CreateTemp hardcodes mode +// 0600. If that is not widened before the rename, the stored asset ends up +// readable only by the CMS's own uid -- the file is there, but the media server +// answers 403 for every uploaded image. Pin the mode so that cannot regress. +func TestStoreUpload_StoresWorldReadableFile(t *testing.T) { + dir := t.TempDir() + + name, size, err := storeUpload(dir, "photo", ".png", bytes.NewReader(pngBytes(t, 4, 4))) + if err != nil { + t.Fatalf("storeUpload: %v", err) + } + if size == 0 { + t.Fatal("storeUpload reported a zero-byte write") + } + + info, err := os.Stat(filepath.Join(dir, name)) + if err != nil { + t.Fatalf("stat stored file: %v", err) + } + // Chmod is not masked by the umask, so this is an exact comparison. + if perm := info.Mode().Perm(); perm != 0o644 { + t.Fatalf("stored file mode = %#o, want 0644 (0600 means Nginx will 403 it)", perm) + } +} + +func TestEnsureTraversable_WidensNarrowDirectory(t *testing.T) { + dir := filepath.Join(t.TempDir(), "2026", "08") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatalf("mkdir: %v", err) + } + + ensureTraversable(filepath.Dir(dir), dir) + + for _, target := range []string{filepath.Dir(dir), dir} { + info, err := os.Stat(target) + if err != nil { + t.Fatalf("stat %s: %v", target, err) + } + if perm := info.Mode().Perm(); perm != 0o775 { + t.Fatalf("%s mode = %#o, want 0775", target, perm) + } + } +}