From 12edb92b6affe99923c81a69829d0d4bf51dfd98 Mon Sep 17 00:00:00 2001
From: Jamie Cameron <jcameron@webmin.com>
Date: Mon, 13 Oct 2025 21:41:06 -0700
Subject: [PATCH] Use the ReadParseMime function properly

---
 updown/lang/en    |  2 +-
 updown/upload.cgi | 71 ++++++++++++++++++++++++-----------------------
 2 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/updown/lang/en b/updown/lang/en
index 6145b4990..0404108d3 100644
--- a/updown/lang/en
+++ b/updown/lang/en
@@ -44,7 +44,7 @@ index_email2=Send email when uploads are done?
 upload_title=Upload Files
 upload_direct=Write files directly to disk?
 upload_err=Failed to upload files
-upload_emove=Failed to move uploaded file from $1 to $2
+upload_emove=Failed to move uploaded file from $1 to $2 : $3
 upload_edir=Missing directory to upload to
 upload_euser=Missing or invalid username
 upload_egroup=Missing or invalid group name
diff --git a/updown/upload.cgi b/updown/upload.cgi
index 4b514669f..01d58a0d4 100755
--- a/updown/upload.cgi
+++ b/updown/upload.cgi
@@ -6,18 +6,19 @@ require './updown-lib.pl';
 &error_setup($text{'upload_err'});
 &ReadParse(\%getin, "GET");
 $upid = $getin{'id'};
-$direct_upload = $getin{'direct'};
-my $tmp;
-# $ENV{'WEBMIN_VAR'} is trully horrible choice to store temporary files
-# but it's only used here for current compatibility with
-# "read_parse_mime_callback". We should fix it right away, on the next commit!!
-$tmp = $ENV{'WEBMIN_VAR'} if ($direct_upload);
-if ($can_mode == 3 && &supports_users()) {
-	# User is fixed
-	@uinfo = getpwnam($remote_user);
-	$tmp = "$uinfo[7]/.tmp" if ($direct_upload);
+
+if ($getin{'direct'}) {
+	# Create a temp directory for the uploaded files for ReadParseMime
+	$direct_dir = &transname();
+	&make_dir($direct_dir, 0711);
 	}
-&ReadParseMime($upload_max, \&read_parse_mime_callback, [ $upid ], 1, $tmp);
+
+if ($can_mode == 3 && &supports_users()) {
+	# User to upload as is set in the ACL
+	@uinfo = getpwnam($remote_user);
+	}
+&ReadParseMime($upload_max, \&read_parse_mime_callback, [ $upid ], 1,
+	       $direct_dir);
 foreach my $k (keys %in) {
         $in{$k} = $in{$k}->[0] if ($k !~ /^upload\d+/);
         }
@@ -51,6 +52,13 @@ $found || &error($text{'upload_enone'});
 &can_write_file($in{'dir'}) ||
 	&error(&text('upload_eaccess', "<tt>$in{'dir'}</tt>", $!));
 
+# If in direct mode, the uploaded files need to be readable by the user we're
+# going to write files as
+if ($direct_dir) {
+	&set_ownership_permissions($uinfo[2], $uinfo[3], undef,
+		$direct_dir, glob("$direct_dir/*"));
+	}
+
 # Switch to the upload user
 &switch_uid_to($uinfo[2], scalar(@ginfo) ? $ginfo[2] : $uinfo[3]);
 
@@ -68,10 +76,18 @@ for($i=0; defined($in{"upload$i"}); $i++) {
 		$d = $in{"upload${i}"}->[$j];
 		$f = $in{"upload${i}_filename"}->[$j];
 		next if (!$f);
+
+		# Work out the short name of the uploaded file, and 
+		# where it is stored if in direct mode
+		$f =~ /([^\\\/]+)$/;
+		my $fname = $1;
 		my $mpath;
+		if ($direct_dir) {
+			$mpath = $direct_dir."/".$fname;
+			}
+
+		# Work out were the uploaded file is being written to
 		if (-d $in{'dir'}) {
-			$f =~ /([^\\\/]+)$/;
-			$mpath = "$tmp/$1" if ($tmp);
 			$path = "$in{'dir'}/$1";
 			}
 		else {
@@ -79,27 +95,17 @@ for($i=0; defined($in{"upload$i"}); $i++) {
 			}
 		print &text('upload_saving',
 			    "<tt>".&html_escape($path)."</tt>"),"<br>\n";
-		# Move file we already have it in disk
+
 		if ($mpath) {
-			if (-w $in{'dir'}) {
-				# Switch back to chown the file
-				&switch_uid_back();
-				chown($uinfo[2], scalar(@ginfo)
-					? $ginfo[2]
-					: $uinfo[3],
-				      $mpath);
-				# Alway move the file as effective user
-				&switch_uid_to($uinfo[2], scalar(@ginfo)
-					? $ginfo[2]
-					: $uinfo[3]);
-				if (!rename($mpath, $path)) {
-					&error(&text('upload_emove',
-						"<tt>$mpath</tt>",
-						"<tt>$path</tt>", $!));
-					}
-				}
+			# Move file as we  already have it in disk
+			rename($mpath, $path) ||
+			  &move_source_dest($mpath, $path) ||
+			    &error(&text('upload_emove',
+				   "<tt>".&html_escape($mpath)."</tt>",
+				   "<tt>".&html_escape($path)."</tt>", $!));
 			}
 		else {
+			# Write uploaded data to the destination file
 			if (!&open_tempfile(FILE, ">$path", 1)) {
 				&error(&text('upload_eopen', "<tt>$path</tt>",
 					$!));
@@ -121,7 +127,6 @@ for($i=0; defined($in{"upload$i"}); $i++) {
 			local $qdir = quotemeta($dir);
 			local $qpath = quotemeta($path);
 			local @files;
-			&switch_uid_back();
 			if ($path =~ /\.zip$/i) {
 				# ZIP file
 				if (!&has_command("unzip")) {
@@ -203,8 +208,6 @@ for($i=0; defined($in{"upload$i"}); $i++) {
 				# Doesn't look possible
 				$err = $text{'upload_notcomp'};
 				}
-			&switch_uid_to($uinfo[2],
-				       scalar(@ginfo) ? $ginfo[2] : $uinfo[3]);
 			if (!$err) {
 				my $jn = join("<br>",
 					      map { "&nbsp;&nbsp;<tt>$_</tt>" }