[Bugfix:InstructorUI] Fix validation of uploaded files

[askadityapandey]

Please check if the PR fulfills these requirements:

• [ ] Tests for the changes have been added/updated (if possible)
• [ ] Documentation has been updated/added if relevant
• [ ] Screenshots are attached to Github PR if visual/UI changes were made

What is the current behavior?

Fixes #10455.

The validateUploadedFiles function in FileUtils.php did not perform checks for file size or filename validity. This could lead to uploading invalid or large files that could cause issues in Submitty.

What is the new behavior?

This pull request modifies the validateUploadedFiles function to perform additional checks for file size, preventing uploads exceeding storage limits, and filename validity using FileUtils::isValidFileName($name) to avoid issues with special characters or reserved names.

Other information?

Due to limitations in my testing environment, I haven't been able to perform comprehensive testing of the changes. However, I've carefully reviewed the code and tested it with simple upload scenarios (valid and invalid files). I'm happy to assist with further testing if the maintainers can provide guidance on setting up a suitable environment.

[bmcutler]

@askadityapandey

Looks like you made cosmetic changes to the indentation for many lines in this file which makes it harder to find and review the critical changes to fix the bug. It may cause merge conflicts with other PRs. Can you please update your PR to only make the changes relevant to the bug fix?

If the indentation of the whole file needs to be refactored to follow formatting conventions that should be done in a PR limited to refactor (not combined with a bugfix or new feature).

Thanks!

[askadityapandey]

Thanks for the quick response, I understand your concern about the indentation in the previous version. I've updated the indentation and focused on highlighting the specific changes made to the validateUploadedFiles function (lines 564-650).

The key changes are the addition of file size and filename validation checks within the validator function (lines 572-630). This includes checking the size against a maximum limit and using FileUtils::isValidFileName to ensure valid filenames.

   public static function validateUploadedFiles(array $files): array {
        if (empty($files)) {
            return ["failed" => "No files sent to validate"];
        }

        $max_size = Utils::returnBytes(ini_get('upload_max_filesize'));

        $validator = function ($file) use ($max_size) {
            $name = $file['name'];
            $tmp_name = $file['tmp_name'];
            $errors = [];

            // Check if temporary file name is empty before calling mime_content_type
            if (empty($tmp_name)) {
                // Handle empty file case (e.g., throw exception or return error message)
                $errors[] = "Empty file uploaded for $name";
            } else {
                $type = mime_content_type($tmp_name);
                $zip_status = FileUtils::getZipFileStatus($tmp_name);

                if ($file['error'] !== UPLOAD_ERR_OK) {
                    $errors[] = ErrorMessages::uploadErrors($file['error']);
                }

                // Check if it's a zip file
                $is_zip = $type === 'application/zip';
                if ($is_zip) {
                    if ($zip_status !== \ZipArchive::ER_OK) {
                        $err_tmp = ErrorMessages::getZipErrorMessage($zip_status);
                        if ($err_tmp != "No error.") {
                            $errors[] = $err_tmp;
                        }
                    } else {
                        $size = FileUtils::getZipSize($tmp_name);
                        if (!FileUtils::checkFileInZipName($tmp_name)) {
                            $errors[] = "Invalid filename within zip file";
                        }
                    }
                }

                // For zip files use the size of the contents in case it gets extracted
                $size = $is_zip ? FileUtils::getZipSize($tmp_name) : $file['size'];

                // Manually check against set size limit in case the max POST size is greater than max file size
                if ($size > $max_size) {
                    $errors[] = "File \"" . $name . "\" too large got (" . Utils::formatBytes("mb", $size) . ")";
                }

                // Check filename
                if (!FileUtils::isValidFileName($name)) {
                    $errors[] = "Invalid filename";
                }
            }

            $success = count($errors) === 0;

            return [
                'name' => $name,
                'type' => $type ?? 'unknown', // Default to 'unknown' if $type is not set
                'error' => $success ? "No error." : implode(" ", $errors),
                'size' => $size ?? 0, // Default to 0 if $size is not set
                'is_zip' => $is_zip ?? false, // Default to false if $is_zip is not set
                'success' => $success,
            ];
        };

        $ret = [];
        if (!is_array($files['name'])) {
            $ret[] = $validator($files);
        } else {
            $num_files = count($files['name']);
            for ($i = 0; $i < $num_files; $i++) {
                // Construct single file from uploaded file array
                $file = [
                    'name' => $files['name'][$i],
                    'type' => $files['type'][$i],
                    'tmp_name' => $files['tmp_name'][$i],
                    'error' => $files['error'][$i],
                    'size' => $files['size'][$i]
                ];
                $ret[] = $validator($file);
            }
        }

        return $ret;
    }

[skara9]

Your PR shouldn't really be touching a lot of code that's not relevant to the changes you're making. Consider reverting your commits and then adding back in the specific code changes you've made. If the indentation is from an auto-formatting tool, probably disable that as well beforehand.

[github-actions[bot]]

This PR has been inactive (no commits and no review comments) for 12 days. If there is no new activity in the next 48 hours, this PR will be labeled as Abandoned PR - Needs New Owner and either another developer will finish the PR or it will be closed.

[williamjallen]

We are unable to show that this fixes the problem described in https://github.com/Submitty/Submitty/issues/10455. @zacharymnp left some comments more than a month ago, but @askadityapandey has not replied. Please file a new PR with the requested changes addressed.