From 87ab726b9f019380f30e30ff8658657fef4848d4 Mon Sep 17 00:00:00 2001 From: Glenn Rice Date: Wed, 2 Apr 2025 22:13:27 -0500 Subject: [PATCH] Rewrite the WeBWorK::Upload module. This takes advantage of things available with Mojolicious. This does not use `Data::UUID` anymore. A comment stated that it was overkill. It is and it was never a good idea to use that for this purpose. This just uses the `Mojo::Asset::Memory::to_file` method which converts to a `Mojo::Asset::File` object and saves the file to a temporary file using `Mojo::File::tempfile` (which uses `File::Temp` under the hood). That is guaranteed to give a safe filename that does not conflict with any existing files. Setting the environment variable `MOJO_TMPDIR` locally to be the passed in directory (which is `$ce->{webworkDirs}{uploadCache}`) ensures that the file is saved in the usual webwork2 upload location (although there is no need for that, and we could drop that configuration variable and just use the system temporary directory for this). The info file that is written is now UTF-8 encoded and decoded. This fixes issue #2690. The `dir` option which was previously required and the only "option" is now just a required argument. It doesn't make sense to use an "option" hash things that are required. Particularly if there is only one option and it is required. Other than what is mentioned above the module behaves much the same. --- lib/WeBWorK.pm | 15 +- lib/WeBWorK/ContentGenerator/Feedback.pm | 4 +- .../Instructor/FileManager.pm | 22 +- lib/WeBWorK/Upload.pm | 323 ++++++------------ 4 files changed, 131 insertions(+), 233 deletions(-) diff --git a/lib/WeBWorK.pm b/lib/WeBWorK.pm index c51daefe96..34a3e92b77 100644 --- a/lib/WeBWorK.pm +++ b/lib/WeBWorK.pm @@ -107,17 +107,14 @@ async sub dispatch ($c) { my @uploads = @{ $c->req->uploads }; - foreach my $u (@uploads) { - # Make sure it's a "real" upload. - next unless $u->filename; - + for my $u (@uploads) { # Store the upload. - my $upload = WeBWorK::Upload->store($u, dir => $ce->{webworkDirs}{uploadCache}); + my $upload = WeBWorK::Upload->store($u, $ce->{webworkDirs}{uploadCache}); - # Store the upload ID and hash in the file upload field. - my $id = $upload->id; - my $hash = $upload->hash; - $c->param($u->name => "$id $hash"); + # Store the upload temporary file location and hash in the file upload field. + my $tmpFile = $upload->tmpFile; + my $hash = $upload->hash; + $c->param($u->name => "$tmpFile $hash"); } # Create these out here. They should fail if they don't have the right information. diff --git a/lib/WeBWorK/ContentGenerator/Feedback.pm b/lib/WeBWorK/ContentGenerator/Feedback.pm index bda5115670..336e5fc127 100644 --- a/lib/WeBWorK/ContentGenerator/Feedback.pm +++ b/lib/WeBWorK/ContentGenerator/Feedback.pm @@ -159,7 +159,7 @@ sub initialize ($c) { my $fileIDhash = $c->param('attachment'); if ($fileIDhash) { my $attachment = - WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), dir => $ce->{webworkDirs}{uploadCache}); + WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), $ce->{webworkDirs}{uploadCache}); # Get the filename and read its contents. my $filename = $attachment->filename; @@ -169,7 +169,7 @@ sub initialize ($c) { local $/; $contents = <$fh>; }; - close $fh; + $fh->close; $attachment->dispose; # Check to see that this is an allowed filetype. diff --git a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm index c81a7d04ca..e634c86991 100644 --- a/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm +++ b/lib/WeBWorK/ContentGenerator/Instructor/FileManager.pm @@ -705,8 +705,7 @@ sub Upload ($c) { return $c->Refresh; } - my ($id, $hash) = split(/\s+/, $fileIDhash); - my $upload = WeBWorK::Upload->retrieve($id, $hash, dir => $c->{ce}{webworkDirs}{uploadCache}); + my $upload = WeBWorK::Upload->retrieve(split(/\s+/, $fileIDhash), $c->{ce}{webworkDirs}{uploadCache}); my $name = $upload->filename; my $invalidUploadFilenameMsg = $c->checkName($name); @@ -779,21 +778,24 @@ sub Upload ($c) { if ($type ne 'Binary') { my $fh = $upload->fileHandle; my @lines = <$fh>; + $fh->close; $data = join('', @lines); if ($type eq 'Automatic') { $type = isText($data) ? 'Text' : 'Binary' } } if ($type eq 'Text') { $upload->dispose; $data =~ s/\r\n?/\n/g; + + my $backup_data = $data; + my $success = utf8::decode($data); # try to decode as utf8 + unless ($success) { + warn "Trying to convert file $file from latin1? to UTF-8"; + utf8::upgrade($backup_data); # try to convert data from latin1 to utf8. + $data = $backup_data; + } + if (open(my $UPLOAD, '>:encoding(UTF-8)', $file)) { - my $backup_data = $data; - my $success = utf8::decode($data); # try to decode as utf8 - unless ($success) { - warn "Trying to convert file $file from latin1? to UTF-8"; - utf8::upgrade($backup_data); # try to convert data from latin1 to utf8. - $data = $backup_data; - } - print $UPLOAD $data; # print massaged data to file. + print $UPLOAD $data; close $UPLOAD; } else { $c->addbadmessage($c->maketext(q{Can't create file "[_1]": [_2]}, $name, $!)); diff --git a/lib/WeBWorK/Upload.pm b/lib/WeBWorK/Upload.pm index ac7df60b9f..a061c722bd 100644 --- a/lib/WeBWorK/Upload.pm +++ b/lib/WeBWorK/Upload.pm @@ -6,275 +6,202 @@ WeBWorK::Upload - store uploads securely across requests. =head1 SYNOPSIS -Given C<$u>, an Mojo::Upload object +Given C<$u>, a C object - my $upload = WeBWorK::Upload->store($u, - dir => $ce->{webworkDirs}->{DATA} - ); - my $id = $upload->id; - my $hash = $upload->hash; + my $upload = WeBWorK::Upload->store($u, $ce->{webworkDirs}{uploadCache}); + my $tmpFile = $upload->tmpFile; + my $hash = $upload->hash; Later... - my $upload = WeBWorK::Upload->retrieve($id, $hash, - dir => $ce->{webworkDirs}->{uploadCache} - ); - my $fh = $upload->fileHandle; - my $path = $upload->filePath; + my $upload = WeBWorK::Upload->retrieve($tmpFile, $hash, $ce->{webworkDirs}{uploadCache}); + my $fh = $upload->fileHandle; + my $path = $upload->filePath; - # get rid of the upload -- $upload is useless after this! - $upload->dispose; + # Get rid of the upload -- $upload is useless after this! + $upload->dispose; - # ...or move it somewhere before disposal - $upload->disposeTo($path); + # Or move it somewhere and dispose of it - $upload is also useless after this! + $upload->disposeTo($path); =head1 DESCRIPTION WeBWorK::Upload provides a method for securely storing uploaded files until such -time as they are needed. This is useful for situations in which an upload cannot -be handled by the system until some later request, such as the case where a user -is not yet authenticated, and a login page must be returned. Since a file upload -should not be sent back to the client and then uploaded again with the user -provides his login information, some proxy must be sent in its place. -WeBWorK::Upload generates a unique ID which can be used to retrieve the original -file. +time as they are needed. This is useful for situations in which an upload needs +to be handled in a later request. WeBWorK::Upload generates a unique temporary +file name and hash which can be used to retrieve the original file. =cut use strict; use warnings; -use Carp qw(croak); -use Data::UUID; # this is probably overkill ;) + +use Carp qw(croak); use Digest::MD5 qw(md5_hex); -use File::Copy qw(copy move); +use Mojo::File qw(path); =head1 STORING UPLOADS -Uploads can be stored in an upload cache -and later retrieved, given the proper ID and hash. The hash is used to confirm -the authenticity of the ID. - -Uploads are Mojo::Upload objects under. - -=head2 CONSTRUCTOR +Uploads can be stored in an upload cache and later retrieved, given the +temporary file name and hash. The hash is used to confirm the authenticity of +the temporary file. -=over +Uploads are constructed from Mojo::Upload objects. -=item store($u, %options) +=head2 store -Stores the Mojo::Upload C<$u> securely. The following keys must be defined in -%options: + my $upload = WeBWorK::Upload->store($u, $dir); - dir => the directory in which to store the uploaded file +Stores the Mojo::Upload C<$u> into the directory specified by C<$dir>. =cut sub store { - my ($invocant, $upload, %options) = @_; + my ($invocant, $upload, $dir) = @_; croak "no upload specified" unless $upload; - # generate UUID - my $ug = new Data::UUID; - my $uuid = $ug->create_str; - - # generate one-time secret - my $secret = sprintf("%X" x 4, map { int rand 2**32 } 1 .. 4); - - # generate hash from $uuid and $secret - my $hash = md5_hex($uuid, $secret); + local $ENV{MOJO_TMPDIR} = $dir; + my $asset = $upload->asset->to_file; + $asset->cleanup(0); + my $tmpFile = path($asset->path)->basename; - my $infoName = "$uuid.info"; - my $infoPath = "$options{dir}/$infoName"; + # Generate a one-time secret. + my $secret = sprintf('%X' x 4, map { int rand 2**32 } 1 .. 4); - my $fileName = "$uuid.file"; - my $filePath = "$options{dir}/$fileName"; - - # get information about uploaded file + # Get the original file name of the uploaded file. my $realFileName = $upload->filename; - # Copy uploaded file - $upload->move_to($filePath); - - # write info file - open my $infoFH, ">", $infoPath - or die "failed to write upload info file $infoPath: $!"; - print $infoFH "$realFileName\n$secret\n"; - close $infoFH; + # Write the info file. + my $infoPath = path($dir)->child("$tmpFile.info"); + eval { $infoPath->spew("$realFileName\n$secret\n", 'UTF-8') }; + die "failed to write upload info file $infoPath: $@" if $@; return bless { - uuid => $uuid, - dir => $options{dir}, - hash => $hash, + tmpFile => $tmpFile, + dir => $dir, + hash => md5_hex($tmpFile, $secret), realFileName => $realFileName, }, ref($invocant) || $invocant; } -=item id +=head2 tmpFile -Return the upload's unique ID, or an undefiend value if the upload is not valid. +Return the temporary file name of the upload, or an undefined value if the +upload is not valid. =cut -sub id { +sub tmpFile { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e "$self->{dir}/$self->{tmpFile}"; - return $uuid; + return $self->{tmpFile}; } -=item hash +=head2 hash -Return the upload's hash, or an undefiend value if the upload is not valid. +Return the hash of the upload, or an undefined value if the upload is not valid. =cut sub hash { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - my $hash = $self->{hash}; - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e "$self->{dir}/$self->{tmpFile}"; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; - - return $hash; + return $self->{hash}; } -=back - =head1 RETRIEVING UPLOADS -An upload stored in the upload cache can be retrieved by supplying its ID and -hash (accessible from the above C and C methods, respectivly. The file -can then be accessed by name or file handle, moved, and disposed of. - -=head2 CONSTRUCTOR - -=over +An upload stored in the upload cache can be retrieved by supplying its temporary +file name and hash (accessible from the above C and C methods), +respectively. The file can then be accessed by name or file handle, moved, and +disposed of. -=item retrieve($id, $hash, %options) +=head2 retrieve -Retrieves the Mojo::Upload referenced by C<$id> and C<$hash>. The following -keys must be defined in %options: + my $upload = WeBWorK::Upload->retrieve($tmpFile, $hash, $dir); - dir => the directory in which to store the uploaded file +Retrieves the upload referenced by C<$tempFile> and C<$hash> and located in +C<$dir>. =cut sub retrieve { - my ($invocant, $uuid, $hash, %options) = @_; - - croak "no upload ID specified" unless $uuid; - croak "no upload hash specified" unless $hash; - - my $infoName = "$uuid.info"; - my $infoPath = "$options{dir}/$infoName"; - - my $fileName = "$uuid.file"; - my $filePath = "$options{dir}/$fileName"; - - croak "no upload matches the ID specified" unless -e $infoPath; + my ($invocant, $tmpFile, $hash, $dir) = @_; - # get real file name and secret from info file - open my $infoFH, "<", $infoPath - or die "failed to read upload info file $infoPath: $!"; - my ($realFileName, $secret) = <$infoFH>; - close $infoFH; + croak 'no upload temporary file name specified' unless $tmpFile; + croak 'no upload hash specified' unless $hash; + croak 'no upload directory specified' unless $dir; - # jesus christ - chomp $realFileName; - chomp $secret; + my $infoPath = path($dir)->child("$tmpFile.info"); - # generate correct hash from $uuid and $secret - my $correctHash = md5_hex($uuid, $secret); + croak 'no upload matches the ID specified' unless -e $infoPath; - #warn __PACKAGE__, ": secret is $secret\n"; - #warn __PACKAGE__, ": correctHash is $correctHash\n"; + # Get the original file name and secret from info file. + my ($realFileName, $secret) = eval { split(/\n/, $infoPath->slurp('UTF-8')) }; + die "failed to read upload info file $infoPath: $@" if $@; - croak "upload hash incorrect!" unless $hash eq $correctHash; + # Generate the correct hash from the $tmpFile and $secret. + my $correctHash = md5_hex($tmpFile, $secret); - # -- you passed the test... -- + croak 'upload hash incorrect!' unless $hash eq $correctHash; return bless { - uuid => $uuid, - dir => $options{dir}, + tmpFile => $tmpFile, + dir => $dir, hash => $hash, realFileName => $realFileName, }, ref($invocant) || $invocant; } -=back - =head2 METHODS -=over - -=item filename +=head3 filename -Returns the original name of the uploaded file. +Returns the original name of the uploaded file, or an undefined value if the +upload is not valid. =cut sub filename { - my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - my $realFileName = $self->{realFileName}; - - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; - - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; + my ($self) = @_; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + # Make sure info file still exists (i.e. the file hasn't been disposed of). + return unless -e "$self->{dir}/$self->{tmpFile}"; - return $realFileName; + return $self->{realFileName}; } -=item fileHandle +=head3 fileHandle -Return a file handle pointing to the uploaded file, or an undefiend value if the -upload is not valid. Suitable for reading. +Return a file handle pointing to the uploaded file suitable for reading, or an +undefined value if the upload is not valid. =cut sub fileHandle { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; + my $filePath = path($self->{dir})->child($self->{tmpFile}); - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e $filePath; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; - - open my $fh, "<", $filePath - or die "failed to open upload $filePath for reading: $!"; + my $fh = $filePath->open('<') or die "failed to open upload $filePath for reading: $!"; return $fh; } -=item filePath +=head3 filePath -Return the path to the uploaded file, or an undefiend value if the upload is not +Return the path to the uploaded file, or an undefined value if the upload is not valid. If you use this, bear in mind that you must not dispose of the upload (either by @@ -285,83 +212,55 @@ wish to move the file, use the C method instead. sub filePath { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; + my $filePath = "$self->{dir}/$self->{tmpFile}"; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e $filePath; return $filePath; } -=item dispose +=head3 dispose -Remove the file from the upload cache. Returns true if the upload was -successfully destroyed, or an undefiend value if the upload is not valid. +Remove the file from the upload cache. =cut sub dispose { my ($self) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; - - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + my $dir = path($self->{dir}); + $dir->child("$self->{tmpFile}.info")->remove; + $dir->child($self->{tmpFile})->remove; - unlink $infoPath; - unlink $filePath; - - return 1; + return; } -=item disposeTo($path) +=head3 disposeTo -Remove the file from the upload cache, and move it to C<$path>. Returns true if -the upload was successfully moved, or an undefiend value if the upload is not -valid. + $upload->diposeTo($path); + +Remove the file from the upload cache, and move it to C<$path>. Returns the +destination as a C object if the upload was successfully moved, or +an undefined value if the upload is not valid. =cut sub disposeTo { my ($self, $newPath) = @_; - my $uuid = $self->{uuid}; - my $dir = $self->{dir}; - croak "no path specified" unless $newPath; + croak 'no path specified' unless $newPath; - my $infoName = "$uuid.info"; - my $infoPath = "$dir/$infoName"; + my $dir = path($self->{dir}); + $dir->child("$self->{tmpFile}.info")->remove; - my $fileName = "$uuid.file"; - my $filePath = "$dir/$fileName"; + my $filePath = $dir->child($self->{tmpFile}); - # make sure info file still exists (i.e. the file hasn't been disposed of) - return unless -e $infoPath; + # Make sure file still exists (i.e. the file hasn't been disposed of). + return unless -e $filePath; - unlink $infoPath; - move($filePath, $newPath); + return $filePath->move_to($newPath); } -=back - -=head1 AUTHOR - -Written by Sam Hathaway, sh002i at math.rochester.edu. Based on the original -WeBWorK::Upload module by Dennis Lambe, Jr., malsyned at math.rochester.edu. - -=cut - 1;