Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions server/internal/handlers/media.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand Down
44 changes: 44 additions & 0 deletions server/internal/handlers/media_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}
Loading