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) + } + } +}