Ideas for enhancement

[dehlirious]

So, I've set SMTP settings for gmail,

No errors at all, so I made a modification in Mailer.php
Adding error_log($th->getMessage()); after catch (Exception $th) { in sendMessage

"PHP message: SMTP Error: Could not authenticate" -> weird.
This issue is on my end, not the codes end but I want to point out:

This was all while reportabuse.php is showing Your report has been submitted successfully

So, log errors in some form + why not display if there was "an internal server error" that prevented the email from sending?

There's no errors if smtp is completely disabled either, so its something to think about

Apparently the issue I was having with authentication is due to this, switching to something like this worked just fine. Sorry for jumping

Also there was an issue I fixed here where the "Add Page"(manage pages) button was not functional due to there being no

element set(even though was set)
Why don't any default 'translations' exist for about/privacy/terms?

It would be kinda nice(but unnecessary ultimately) to be able to add 'allowed' file extensions beyond editing vendor/farisc0de/phpfileuploading/src/filter.json

• Why are file names hashed? E.g. 'mantle.html' to '5d339950900db9ceb5cc40ae367419b44575a084.html' , is this something that can be a setting? Where is the component that changes the name? I'd definitely enjoy to modify it myself for my own instance but I haven't found it,
  I know of the functionality inside /src/Upload.php but I haven't narrowed it down to find the exact piece yet

[farisc0de]

1. Glad that worked.
2. I'll look into the add page button issue
3. Yeah actually I was planning to do so to edit the filter from the dashboard
4. Yes, you can find this setting in actions/upload_file.php line 54 u can remove this $upload->hashName(); and it will save the file with the original name

[dehlirious]

Again, I'm a big fan so far of the project. I'm definitely gonna work on it for a little and try and get it to a point where I feel comfortable releasing it to my site, adding and changing stuff to my own liking.

Have you ran into any issues with having the original file names saved? Definitely a situation where you'd want to escape the name just for safety. Mainly I was wondering if you chose hashed file names for a compatibility/security reason or out of design choice.

Have you considered having a "instances" section in the readme.md , able to show public instances running this software? (If people make a pull request to have their website/instance added anyway) - just a quick thought/question , I've seen it done with LibreY and I liked the concept personally.

What's your preferred method of contact if you don't mind? I'd enjoy discussing certain ideas/questions as they come up in a non public space, if you're up for it. Twitter, Discord, Whatsapp?

[farisc0de]

For the hashing it is there to upload the same file more than once because I am generating a new file id for each upload.

I am might look for a better method to handle this.

For other contact channels u can try me on Discord fariscode.

[dehlirious]

For the hashing it is there to upload the same file more than once because I am generating a new file id for each upload.

I am might look for a better method to handle this.

That makes sense.

I'm thinking of having the file saved as a hashed version, but saving the filename in plaintext in the database to show / download as.

I also came up with this but it doesn't seem ideal for me, just functional

foreach ($_FILES['file'] as $key => $file) {
    if ($key === 'name' && !empty($file)) {
        $_FILES['file'][$key] = pathinfo($file, PATHINFO_FILENAME) . '_' . generateRandomString(6) . '.' . pathinfo($file, PATHINFO_EXTENSION);
    }
}

atm, I'm looking into just having hash_id as the file name on disk, as that would not have duplicate files (for the same user) to exist and allow file_name to be the actual file name without the issue you brought up

[dehlirious]

What're your thoughts on implementing this

MIME/File Extension Protection is optional, change via admin dashboard
Enable/Disable Twitter/LinkedIn/Instagram links in the footer
A change to updateSettings, beneficial for testing purposes otherwise has no affect on functionality
ee1ea5f
Forgot to include this change, it is also included in the above commit 6b75aae

(allows users to change theme without being signed in)
2782069

And for now, I think I'm probably just going to privately work on my instance until I'm done. It's a headache to try and work with github, keeping my local changes up to date with my repository, the fact that I cannot make pull requests with specific commits but the entire branch of changes is also frustrating.

I'd also have to suggest, in installLogic.php to add something to either move or delete install.php once installation is done. Probably moving the file, renaming it to have random characters suffixed would suffice.

on download.php, could change the existing method of save qrcode to db to
<img class="mr-auto justify-content-center rounded" src="<?= "https://quickchart.io/qr?text=" .SITE_URL . "/download.php?file_id=" . $file_data->file_id . "&size=150" ?>" title="QR Code" /> (ideally , less stored in the database the better)
(i'm considering removing 'downloadlink' 'directlink' 'deletelink' 'editlink' with the same type of alternatives but that'll be later on if I do)

-- suggestion(I may work on this myself):
Add the edit.php variables directly to the upload page, so when a user uploads a file they can then determine whether they want it to expire after X amount of days, X amount of downloads, etc.
This is also necessary because 'guest' users (not signed in, when public uploads is enabled) do not have a way to set expirations

To-Do(on my end):
Allow for file name's to remain unmodified(not a hashed version) + allow for file extensions like .php to be uploaded without executing them

Currently, if I upload a file like
moible.js_lWsEWv.txt and try and download it, it will result in text/plain, which will not download.
If I uploaded a .php file, and tried to download it, it would execute

The filter.json is to prevent that but I'm going an alternative route that allows uploading of any file type without security concern.

[farisc0de]

1. For me no, as as it will open the application for security issues. like uploading php files.
2. This is actually a good one enabling what smc you have.
3. I'll look into it.
4. Yes that is a good idea and i'll look into also, I might add some kind of checker before showing the installation page
5. Actully all these data I pulling them from the package, I build it to be a one stop shop for the all the functions in the software.
6. Yeah i'll check that's correct now only singed in user can edit the limits

[dehlirious]

1. For me no, as as it will open the application for security issues. like uploading php files.
2. This is actually a good one enabling what smc you have.
3. I'll look into it.
4. Yes that is a good idea and i'll look into also, I might add some kind of checker before showing the installation page
5. Actully all these data I pulling them from the package, I build it to be a one stop shop for the all the functions in the software.
6. Yeah i'll check that's correct now only singed in user can edit the limits

Fair. The comment I made after this, implementing "dl.php" is my solution. Because yeah, right after I enabled the "disable mime protection" I thought about it, uploaded a PHP file and sure enough, I hacked myself.

Now I made that whole thing that rewrites the extension + doesn't allow the user direct access to the file itself, to Now be able to properly allow .PHP file uploads without security concern

And a checker before showing the install page is also a good idea lol. Only allow it to be shown if the environment mode is installation perhaps?

And the one stop shop idea makes sense , I'm just prone to customizing things for my own exact usage, optimization and security wise lol

Also, what did you mean smc , I'm just waking up but that word is undefined in my dictionary

[dehlirious]

Currently, if I upload a file like
moible.js_lWsEWv.txt and try and download it, it will result in text/plain, which will not download.
If I uploaded a .php file, and tried to download it, it would execute

Solution: (Sorry these aren't packaged into a nice commit for you!)

This does not cover the database changes, a column with the name file_name needs to exist within the files table
(I was going to go the route of changing file_data instead but modifying your Upload.php in your phpfileuploading package was undesirable beyond what I've already done)

This is also potentially crude, I'm okay with it the way it is but you may want to pretty it up a bit in your own way before implementing, or change it to your style(e.g. all that I've added isn't within a class construct yet or anything, I'll probably develop this further but I was excited to share.

This isn't the latest version of changes on my end, this is rather meant to be a guide as to how you can implement such a concept. Do with it what you will!

UploadHandler.php

    public function addFile($file_id, $file_name, $user_id, $file_data, $user_data, $file_settings)
    {
        $this->db->prepare(
            "INSERT INTO files (
                file_id,file_name,user_id,file_data,user_data,file_settings,uploaded_at) 
                VALUES
                (:file_id,:file_name,:user_id,:file_data,:user_data,:file_settings,:uploaded_at)"
        );

        $data = json_decode($file_data);

        $this->db->bind(":file_id", $file_id, \PDO::PARAM_STR);
        $this->db->bind(":file_name", $file_name, \PDO::PARAM_STR);
        $this->db->bind(":user_id", $user_id, \PDO::PARAM_STR);
        $this->db->bind(":file_data", $file_data, \PDO::PARAM_STR);
        $this->db->bind(":user_data", $user_data, \PDO::PARAM_STR);
        $this->db->bind(":file_settings", $file_settings, \PDO::PARAM_STR);
        $this->db->bind(":uploaded_at", $data->uploaddate, \PDO::PARAM_STR);

        return $this->db->execute();
    }

Upload.php (inside the phpfileuploading package)

	public function getFileName(){
		return $this->file_name;
	}

upload_file.php

// remove  $upload->hashName();
//BEFORE $upload->setUpload
	foreach ($_FILES['file'] as $key => $file) {
		if ($key === 'name' && !empty($file)) {
			$_FILES['file'][$key] = $_FILES['file']['name'] . '.rewrittenextension' . generateRandomString(8);
		}
	}

...redacted...
$handler->addFile(
            $upload->getFileID(),
			getValidatedOriginalFilename($upload->getFileName(), $baseExtension),
            $upload->getUserID(),
...redacted...

download.php
Change <a href="<?= $file_data->directlink ?>" class="btn btn-primary text-decoration-none" download> to
<a href="<?= 'dl.php?url=' . $URLDL . '&file_id=' . $file->file_id ?>" class="btn btn-primary text-decoration-none" download>

downloadLogic.php (at the end)
$URLDL = base64_encode(encryptURL($file->user_id . '/' . $file_data->filename, $secretKey));

myFilesLogic.php

foreach ($files as $file) {
    $decodedData = json_decode($file->file_data, true);
    // Add the 'file_name' property to the decoded data
    $decodedData['file_name'] = $file->file_name;
    $files_info[] = $decodedData;
}

my_files.php
Change <td><?= $file['filename']; ?></td>
to
<td><?= isset($file['file_name']) ? $file['file_name'] : $file['filename']; ?></td>

admin/session.php
add include_once dirname(__FILE__, 2) . '/src/utils.php'; (or whatever file holds the custom functions down below)

admin/logic/homeLogic.php

foreach ($latest as $file) {
    $decodedData = json_decode($file->file_data, true);
    // Add the 'file_name' property to the decoded data
    $decodedData['file_name'] = $file->file_name;
    $files_info[] = $decodedData;
}

admin/index.php
Change <td><?= $file['filename']; ?></td> to <td><?= isset($file['file_name']) ? $file['file_name'] : $file['filename']; ?></td>

admin/files/logic/viewLogic.php

foreach ($files as $file) {
    $decodedData = json_decode($file->file_data, true);
    // Add the 'file_name' property to the decoded data
    $decodedData['file_name'] = $file->file_name;
    $files_info[] = $decodedData;
}

admin/files/view.php
Change <td><?= $file['filename']; ?></td> to <td><?= isset($file['file_name']) ? $file['file_name'] : $file['filename']; ?></td>

Add these function's (I made a new file called utils.php in 'src' and included it in session.php under init.php

function generateRandomString($length,$prefix="",$suffix="",$specialChars=false){$lowercaseChars='abcdefghijklmnopqrstuvwxyz';$uppercaseChars='ABCDEFGHIJKLMNOPQRSTUVWXYZ';$numericChars='0123456789';$specialCharsSet=$specialChars?'!@#$%^&*()-_+=~[]{}\\|<>?/':'';$availableChars=$lowercaseChars.$uppercaseChars.$numericChars.$specialCharsSet;$randomString='';$length=max(1,min($length,1000));for($i=0;$i<$length;$i++){$randomString.=$availableChars[mt_rand(0,strlen($availableChars)-1)];}$extraChars='';for($i=0;$i<4;$i++){$extraChars.=$availableChars[random_int(0,strlen($availableChars)-1)];}$middleIndex=intval($length/2);$randomString=substr($randomString,0,$middleIndex).$extraChars.substr($randomString,$middleIndex);if(is_numeric($randomString[0])){$randomString[0]=$lowercaseChars[random_int(0,strlen($lowercaseChars)-1)];}while(strlen($randomString)>$length){$randomString=substr($randomString,0,-1);}$randomString=$prefix.$randomString.$suffix;return $randomString;}

// Function to check if the filename ends with the base extension followed by any characters
function hasDynamicRewrittenExtension($filename, $baseExtension) {
    // Split the filename by "." to isolate the extension
    $parts = explode('.', $filename);

    // If there's only one part, it means there's no extension
    if (count($parts) < 2) {
        return false;
    }

    // The last part is the extension
    $extension = array_pop($parts);

    // Check if the extension starts with the base extension
    return strpos($extension, $baseExtension) === 0;
}

// Base part of the rewritten extension
$baseExtension = 'rewrittenextension';

// Function to remove the dynamic extension from the filename, if present
function getValidatedOriginalFilename($filename, $baseExtension) {
    // Split the filename by "." to separate extensions
    $parts = explode('.', $filename);

    // If there's only one part, it means there's no extension to remove
    if (count($parts) < 2) {
        return $filename;
    }

    // Assume the last part is the extension
    $extension = array_pop($parts);

    // Check if the extension starts with the base extension and remove it
    if (strpos($extension, $baseExtension) === 0) {
        // Rebuild the filename without the last extension
        $filenameWithoutExtension = implode('.', $parts);
    } else {
        // If the last part is not the dynamic extension, add it back
        array_push($parts, $extension);
        $filenameWithoutExtension = implode('.', $parts);
    }

    return $filenameWithoutExtension;
}

function encryptUrl($data, $secretKey) {
    $method = 'AES-256-CBC';
    $ivLength = openssl_cipher_iv_length($method);
    $iv = openssl_random_pseudo_bytes($ivLength);

    $encrypted = openssl_encrypt($data, $method, $secretKey, 0, $iv);
    // Combine IV with encrypted data for storage
    $encryptedWithIv = base64_encode($iv . $encrypted);

    return $encryptedWithIv;
}

function decryptUrl($data, $secretKey) {
    $method = 'AES-256-CBC';
    $dataWithIv = base64_decode($data);

    $ivLength = openssl_cipher_iv_length($method);
    $iv = substr($dataWithIv, 0, $ivLength);
    $encryptedData = substr($dataWithIv, $ivLength);

    $decrypted = openssl_decrypt($encryptedData, $method, $secretKey, 0, $iv);

    return $decrypted;
}

// Example usage
$secretKey = 'your-256-bit-secret';

dl.php

include_once('session.php'); //ASSUMING** That session.php has src/utils(our custom file) included in it, if not include it below this line

// Define the base directory where files are stored
$baseDir = 'uploads/';

// Define the restricted directory
$restrictedDir = 'uploads/settings/';

// Get the file path from the GET parameter
$filePath = isset($_GET['url']) ? $_GET['url'] : '';

$realPath = decryptUrl(base64_decode($filePath), $secretKey); //if not base64 wrapped, failure to download occurs in certain circumstances!

// Security: Prevent directory traversal
$realBase = realpath($baseDir);
$realUserPath = realpath($baseDir . $realPath);
$realRestrictedDir = realpath($restrictedDir);

// Ensure the file exists and is within the "uploads" directory but not within the restricted directory
if ($realUserPath === false || strpos($realUserPath, $realBase) !== 0 || !file_exists($realUserPath)) {
    die('File not found or access denied.');
}

// Check if the requested file is inside the restricted directory
if ($realRestrictedDir !== false && strpos($realUserPath, $realRestrictedDir) === 0) {
    die('Access to the requested directory is forbidden.');
}

// Validate the extension and get the original filename
$originalFilename = getValidatedOriginalFilename(basename($realUserPath), $baseExtension);

// Specify headers for chunked transfer
header('Content-Type: application/octet-stream');
header('Content-Disposition: attachment; filename="' . $originalFilename . '"');
header('Expires: 0');
header('Cache-Control: must-revalidate');
header('Pragma: public');
header('Content-Length: ' . filesize($realUserPath));

// Flush system output buffer
flush();

// Open the file
$handle = fopen($realUserPath, 'rb');

// Check if the file is opened successfully
if ($handle === false) {
    die('Cannot open file for reading.');
}

// Transfer the file in chunks to minimize memory usage
while (!feof($handle)) {
    echo fread($handle, 1 * 1024 * 1024); // Read in 1MB chunks
    flush(); // Flush the output buffer
}

// Close the file handle
fclose($handle);

exit;

Now, why have the url encrypted you might ask?
Because if the user knew exactly where the file was located, the exact file extension, they would simply visit it directly. Renaming the file extension in of itself is a solution to allow files like .php to be downloaded without being executed, but I still decided to take it a step further and relocate all download requests to dl.php for an additional layer of security. (Technically speaking, the downloader not knowing the user_id of the user is a bit ideal in my eyes)

isset($file['file_name']) ? $file['file_name'] : $file['filename']; < this logic adds some layer of backwards compatibility, so if a file_name column entry doesn't exist, it'll just show the original filename.

To fix download count (it always stays null)

dl.php add (below session.php)

$db = new Uploady\Database;
$handler = new Uploady\Handler\UploadHandler($db);
if (isset($_GET['file_id'])) {
    if ($handler->fileExist($_GET['file_id'])) {
		$file = $handler->getFile($_GET['file_id']);
		$file_data = json_decode($file->file_data);
		$file_settings = json_decode($file->file_settings, true);
		
		//Increment download count
		$db->prepare('UPDATE files SET downloads = COALESCE(downloads, 0) + 1 WHERE file_id = (:file_id) LIMIT 1;');
		$db->bind(":file_id", $_GET['file_id'], \PDO::PARAM_STR);
		
		$db->execute();
        
    }
}

downloadLogic.php, remove $handler->addDownload($_GET['file_id']);

I'm pretty happy to have also made the download count functional! Before , downloads always stayed at NULL, now they increment with every download. (A work around is to remove file_id from the GET params but that's not a huge cause for concern, I hope)
I did have to remove the call to addDownload though, as merely visiting the page and not downloading shouldn't count as a download, combined with my download increment method and it caused +2 for every download instead of +1

Alternatively, keep your existing method but at least fix addDownload(UploadHandler.php), change (downloads + 1) to COALESCE(downloads, 0) + 1

[dehlirious]

Another suggestion, less storage used in the database which is essential for big operations

To be able to remove qrcode/directlink/deletelink/editlink in getJSON()

In my_files.php

<td class="text-center">
    <ul class="list-inline">
        <li class="list-inline-item">
            <a href="<?= SITE_URL . "/download.php?file_id=" . $file['file_id']  ?>">
                <i class="fa fa-download" aria-hidden="true"></i>
            </a>
        </li>
        <li class="list-inline-item">
            <a href="<?= SITE_URL . "/edit.php?file_id=" . $file['file_id'] . "&user_id=" . $file['user_id'] ?>">
                <i class="fa fa-edit" aria-hidden="true"></i>
            </a>
        </li>
        <li class="list-inline-item">
            <a href="<?= SITE_URL . "/delete.php?file_id=" . $file['file_id'] . "&user_id=" . $file['user_id']  ?>">
                <i class="fa fa-trash" aria-hidden="true"></i>
            </a>
        </li>
    </ul>
</td>

In download.php
<img class="mr-auto justify-content-center rounded" src="<?= "https://quickchart.io/qr?text=" .SITE_URL . "/download.php?file_id=" . $file_data->file_id . "&size=150" ?>" title="QR Code" />
($file_data->directlink in thiss file was already removed by my previous changes)

editFilesLogic.php
$picture = 'dl.php?url=' . base64_encode(encryptURL($file->user_id . '/' . $file_data['filename'], $secretKey));

qrcode/directlink/deletelink/editlink are not used anywhere else in the project to my knowledge

Issues to fix: Downloadlink/deletelink temporarily stays until I fix the javascript aspect, which requires it!

[dehlirious]

Hey, with this,
06b954b#diff-31aebea8da7e6ca01ed506541ac9684b2806863656d58c5e3d9e9cca5f655495

Why'd you change function isExist return false to return true? I would undo that change personally, the result comes from return $this->isExpired($token) ? false : true; which is proper, and if $this->db->execute() Doesn't execute, we would want to return false, rather than true

[dehlirious]

Please remove chunking: true, from upload.js ! With the way the code currently is, this is invalid! Say, if I upload a 16mb file, it now uploads 9 different 'chunked' files rather than one single file.

I stated

Also a way to have this functional would be ideal

Sorry for not clarifying as to what exactly happened if you actually added chunking: true to the upload.js file !

And me personally, I would increase the maxFilesize limit to 1gb, the upload software will reject the upload if they're not allowed to have 1gb uploads but if they Are allowed 1gb uploads, that 250 limit won't let them upload anything past 250mb

[dehlirious]

Also these still exist

Another bug: Warning: Undefined array key "delete_files_success" in profile/my_files.php on line 23

(delete_files_success doesn't exist in en.json)

Another: if you input a invalid file_id on edit.php, many warnings instead of a proper handling of the issue(Attempt to read property "file_data" on false) whereas delete.php handles improper file_id correctly

[farisc0de]

Fixed also the issue with the DropZone max size is fixed it is configured through the size in config.php

[dehlirious]

e56f64b#diff-31aebea8da7e6ca01ed506541ac9684b2806863656d58c5e3d9e9cca5f655495

The switch in isExpired reverts the previous fix?
Now it'll respond false if it is expired and true otherwise, re-breaking the functionality (unless you remove ? false : true; in isExist which would then fix it technically)

In my prior comment regarding 06b954b#diff-31aebea8da7e6ca01ed506541ac9684b2806863656d58c5e3d9e9cca5f655495 I was just saying to revert the change in isExist explicitly.

This is what I was meaning

    public function isExist($token)
    {
        $this->db->prepare("SELECT * FROM users WHERE reset_hash = :token");

        $this->db->bind(":token", sha1($token), \PDO::PARAM_STR);

        if ($this->db->execute()) {
            if ($this->db->rowCount()) {
                return $this->isExpired($token) ? false : true;
            }
        }

        return false;
    }
    public function isExpired($key)
    {
        $this->db->prepare("SELECT created_at FROM users WHERE reset_hash = :token");

        $this->db->bind(":token", sha1($key), \PDO::PARAM_STR);

        if ($this->db->execute()) {
            $data = $this->db->single();

            $diff = time() - strtotime($data->created_at);

            if (round($diff / 3600) >= 24) {
                $this->deleteToken($key);
                return true;
            }

            return false;
        }

        return false;
    }

[dehlirious]

Yet another suggestion

Move this snippet to before $username = isset($_SESSION['username']) ? $_SESSION['username'] : null; to allow guests to change their theme

    $theme = $_GET['theme'] ?? $_SESSION['theme'] ?? 'light';
    if ($theme == 'dark') {
        $_SESSION['theme'] = 'dark';
        $theme = 'dark';
    } else {
        $_SESSION['theme'] = 'light';
        $theme = 'light';
    }

[dehlirious]

In terms of the latest sha1 to sha256 ,

Upload.php + File.php in phpfileuploading all still use sha1, not sure if this is of a concern for you or not. Seems not too important to change it around but I figure I may as well point it out

[farisc0de]

Yep, I am also upgrading the package to use SHA256, I am using now SonarCloud to detect any bugs or any vulnerability I am not aware of, one of them was the SHA1.

[dehlirious]

Found a (security) issue!

There's no authentication that the user-id matches the session user-id, meaning if you have a file-id and user-id you can delete the files of another user

Solution:
Modify deleteLogic.php
Replace

if ($handler->fileExist($_GET['file_id']) && $handler->userExist($_GET['user_id'])) {

with

if ($handler->fileExist($_GET['file_id']) && $handler->userExist($_GET['user_id']) && $_SESSION['user_id'] == $_GET['user_id']) {

*modify to your hearts discretion, I imagine you'll use something out of lang $lang["general"]["file_or_user_not_found"] and instead of die

[dehlirious]

That's cool! I've never used SonarCloud before, maybe I'll have to look into it or use something similar to try and find bugs

Edit: This seems cool! Few false positives but it's neat.

Insignificant potential XSS threats:

reportabuse.php (remove xss & warning if file_id isn't set)
$_GET['file_id'] -> isset($_GET['file_id']) ? $utils->sanitize($_GET['file_id']) : ''

Everything in the admin panel should be not worried about but screw it.
admin/pages/edit.php
$_GET['pageid']; -> isset($_GET['pageid']) ? $utils->sanitize($_GET['pageid']) : ''

admin/pages/logic/editLogic.php
$slug = $page->getSlug($_GET['pageid'])->slug; -> $slug = isset($_GET['pageid']) ? $page->getSlug($_GET['pageid'])->slug : null;

admin/languages/edit.php
$_GET["lang"]; -> isset($_GET['lang']) ? $utils->sanitize($_GET['lang']) : ''

quick fix to avoid a couple warnings if trying to modify a language that doesn't exist

PHP Warning:  file_get_contents(uploady//languages/ab.json): Failed to open stream: No such file or directory
Warning: Trying to access array offset on null 
Warning: foreach() argument must be of type array|object, null given

<h3>General</h3>
<?php if (isset($language['general']) && is_array($language['general'])) : ?>
    <?php foreach ($language['general'] as $key => $value) : ?>
        <label><?= $key; ?></label>
        <input class="form-control" type="text" name="general_<?= $key ?>" value="<?= $value ?>">
    <?php endforeach; ?>
<?php else : ?>
    <b>No language found</b>
<?php endif; ?>

<h3>Navbar</h3>

<?php if (isset($language['navbar']) && is_array($language['navbar'])) : ?>
    <?php foreach ($language['navbar'] as $key => $value) : ?>
        <label><?= $key ?></label>
        <input class="form-control" type="text" name="navbar_<?= $key ?>" value="<?= $value ?>">
    <?php endforeach ?>
<?php else : ?>
    <b>No language found</b>
<?php endif ?>

<h3>Theme</h3>

<?php if (isset($language['theme']) && is_array($language['theme'])) : ?>
    <?php foreach ($language['theme'] as $key => $value) : ?>
        <label><?= $key ?></label>
        <input class="form-control" type="text" name="theme_<?= $key ?>" value="<?= $value ?>">
    <?php endforeach ?>
<?php else : ?>
    <b>No language found</b>
<?php endif ?>

This does not fix the issues in Localization.php , file_get_contents without checking if the file exists, but I'll leave that for later!
(Also using "APP_PATH . '/'" causes // since app_path already includes a '/' at the end, this issue is also spread out amongst too many files to tame right now)

Back to issues found by the vulnerability scanner:

Utils.php

setcookie misses the HttpOnly attribute (it is false by default). Set it to true to protect the cookie from possible malicious code on client side.

createCookie
return setcookie($name, $value, time() + 60 * 60 * 24 * 30, "/"); -> return setcookie($name, $value, time() + 60 * 60 * 24 * 30, "/", "", false, true);
deleteCookie
return setcookie($name, "", time() - 3600, "/"); -> return setcookie($name, "", time() - 3600, "/", "", false, true);

Functionality issue that needs fixed:
in downloadLogic.php , uploads folder is defined statically instead of using the UPLOAD_FOLDER env parameter
On download count reaching it's limit,
PHP Warning: Undefined array key "user_id" in/logic/downloadLogic.php

The proper alternative would be to use {$file->user_id} instead of {$_GET['user_id']}

Now Undefined property: stdClass::$filename in /logic/downloadLogic.php
change $file to $file_data

So the result is
unlink(realpath(UPLOAD_FOLDER . "/{$file->user_id}/{$file_data->filename}"));

Tested, and it is functional!

in deleteLogic.php , it is handled a little bit differently so no big issue but we should sanitize out of caution (even though logically nothing can happen, lets do it anyway) + we have to implement UPLOAD_FOLDER

if (isset($_GET['file_id']) && isset($_GET['user_id'])) {
	$fileID = $utils->sanitize($_GET['file_id']);
	$userID = $utils->sanitize($_GET['user_id']);
	
    if ($handler->fileExist($fileID) && $handler->userExist($userID) && $_SESSION['user_id'] == $userID) {
        $file = json_decode($handler->getFile($fileID)->file_data);
		
        if ($handler->deleteFile($fileID, $userID)) {
            unlink(realpath(UPLOAD_FOLDER . "/{$userID}/{$file->filename}"));
            $msg = $lang["general"]['file_deleted_success'];
        } else {
            $msg = $lang["general"]['file_deleted_failed'] . " ):";
        }
    } else {
        $msg = $lang["general"]["file_or_user_not_found"];
    }
} else {
    $msg = $lang["general"]['file_id_missing'];
}

$title = $lang["general"]['delete_file_title'];
$page = 'delete_file';

And lets amend delete_file.php

if (isset($_POST['file_id']) && isset($_POST['user_id'])) {
	$fileID = $utils->sanitize($_POST['file_id']);
	$userID = $utils->sanitize($_POST['user_id']);
	
    if ($handler->fileExist($fileID) && $handler->userExist($userID)) {
        $file = json_decode($handler->getFile($fileID)->file_data);
        if ($handler->deleteFile($fileID, $userID)) {
            unlink(realpath("../" . UPLOAD_FOLDER . "/{$userID}/{$file->filename}"));

To-do (in my mind) make a function for unlinking/deleting files that prevents a path travel attack. Likely to not be a problem but considering that there's 3+ files that handle the deletion of files, this should be implemented I imagine.

[dehlirious]

Weird!
I encountered a bit of a bug,

Any form of "Updating password" results in being unable to login with that exact password used. Forget password + update account results in the same issue, of not being able to sign back in

EDIT:
PHP Deprecated: password_hash(): Passing null to parameter #1 ($password) of type string is deprecated profile/actions/update.php

in profile/actions/update.php
$password = $utils->sanitize($_POST['Password']);
its $_POST['password'] not $_POST['Password'] , that was the issue as to why I couldnt update my password

After making that change, password updating worked

BUT, i'm not clear and out of the water yet unfortunately. Resetting password doesn't work

Because, the password is not sanitized before being sent to the database

        $Password = $utils->sanitize($_POST['password']);
        $confirmPassword = $utils->sanitize($_POST['confirmPassword']);
        if ($Password == $confirmPassword) {
            if ($updatePassword->updatePassword($token, $data->username, $_POST['password'])) {

Update $_POST in updatePassword to $utils->sanitize($_POST['password']) and it works

And another bug, password_pattern_msg doesn't exist in en.json
PHP Warning: Undefined array key "password_pattern_msg" in uploady/reset.php

Hurray!! I've fixed my head scratching issue, this gave my brain an ache for a minute until I figured it out.

One more potential issue: (not really an issue but): On reset password page, you can enter either username or email, whereas the form specifies "Email address"
Either specify username or email, or force email only.
<input type="text" class="form-control" name="email" placeholder="Enter Email">
Update type to be of email if you choose the email route

I may update my Instance to use PASSWORD_ARGON2I rather than BCRYPT in terms of password_hash, and also im considering sha3-256 over sha-256. Overkill but , I'll research and see if there's a downside. If not, why not.

[dehlirious]

Issue:

admin/users/actions/new.php

$password = $utils->sanitize($_POST['password']);
$_POST['password'] = password_hash($password, PASSWORD_BCRYPT);

instead of

$_POST['password'] = password_hash($_POST['password'], PASSWORD_BCRYPT);

Also, I don't know if you'd be interested but I've implemented it in my own instance

This function prioritizes Argon2ID over bcrypt, when it's available.

Dynamically calculates memory, cost, and threads based on the current system's available resources.

Overall , it's fairly good. Give me your thoughts if you can.

https://github.com/dehlirious/php_argon2id/blob/main/argon2id.php

Here's how it operates:

• The baseline for memory usage starts at 128MB. If the system allows, this can be increased up to three times, maxing out at 384MB for password hashing operations. This ensures the function can make the most of available resources without exceeding practical limits. (This can be configured. Likely 128mb is enough) - available ram is decided via memory_limit in php.ini + used ram

• With a starting point of 4 threads and 4 iterations, the function scales up to 6 iterations and 8 threads for the hashing process. This scale is based on a deliberate decision that sees no additional value in surpassing the 384MB mark, balancing execution time and security effectively.

• Execution times vary based on the configuration, with 482ms for the highest setting (384MB RAM, 6 iterations, 8 threads) and 100ms at the baseline (128MB RAM, 4 iterations, 4 threads). Systems with 200MB of available RAM also run at 4 iterations and 4 threads, resulting in a 168ms execution time.

• The correlation of iteration and thread count with available RAM is a design choice to match the function's demand with the system's capabilities, ensuring efficiency across a spectrum of hardware configurations. This approach avoids overloading less powerful machines while utilizing the full potential of higher-end systems.

• The function defaults to BCRYPT if the calculated memory cost falls below 24MB or if Argon2id is not available, maintaining compatibility across various platforms and configurations.

The function's adaptability makes it a solid choice for secure password hashing, providing scalability and robustness for a wide range of application scenarios.

in terms of picking Argon2id parameters, the correct order is:
First increase memory usage to as much as possible with a time cost parameter of 1**.
Increase the parallelism as much as still possible without lowering the previous parameter, until you hit the number of cores your system has.
Increase the time cost parameter as much as still possible without lowering the previous parameters.

What's left to do(maybe): On signin, run password_needs_rehash. Technically speaking, having it in the function above is pointless but I added it regardless. The correct way would be, on each signin run password_needs_rehash to determine if the stored hashed password needs to be rehashed, then rehash if necessary. I'll make a function for this later.

If you wanted to implement it, you'd have to modify
Line 22 of admin/users/actions/new.php
Line 20 of admin\users\actions\update.php
Line 289 of logic\installLogic.php
Line 37 of logic\signupLogic.php
Line 19 of profile\actions\update.php
Line 155 of src\Uploady\ResetPassword.php

hashPassword($password)

Caution
Using the PASSWORD_BCRYPT as the algorithm, will result in the password parameter being truncated to a maximum length of 72 bytes.

Argon2 is the winner of the Password Hashing Competition (PHC) and is designed to be memory-hard and resistant to various attacks, including brute-force and side-channel attacks.
Argon2 offers several advantages over bcrypt, including better resistance to GPU-based attacks and the ability to tune parameters such as memory cost, time cost, and parallelism to adapt to evolving hardware capabilities.
Argon2 comes in two variants: Argon2ID and Argon2I. Argon2ID is recommended for most use cases as it combines Argon2I and Argon2D, providing hybrid resistance against side-channel attacks.
While Argon2 offers excellent security, it may be slightly more computationally expensive than bcrypt, which could impact performance in high-load scenarios.

[dehlirious]

Another fix for you!

reportAbuseLogic.php
$fileID = $utils->sanitize($_POST['fileid']) ?? null;

Warning: Undefined array key "fileid"

Fix it by
$fileID = isset($_GET['file_id']) ? $utils->sanitize($_GET['file_id']) : null;
$_POST -> $_GET , fileid -> file_id

And in actions/reportabuse.php
Declare this

$fileID = $utils->sanitize($_POST['fileid']);

Replace usage of $_POST['fileid'] with $fileID, alongside sanitizing email + reason (no harm in doing that)

Yet another oddball issue!

In session.php,

if (isset($_SESSION)) {

    $username = isset($_SESSION['username']) ? $_SESSION['username'] : null;

$username is always null, because $_SESSION['username'] is never set.
die(var_dump($_SESSION)); to test this theory. which confuses me because in logic/loginLogic.php , $_SESSION['username'] = $username; should've set it

And because $username is never set,
if ($username != null) { in session.php never gets ran, so variables like $_SESSION['current_ip'] never get set nor $_SESSION['csrf']

But yet $_SESSION['user_id'] is set.

Kindof confusing, I don't have enough time to dig into it tonight lol.

reportabuse.php
<input ..... name="fileid" ..... readonly> < consider either making this input hidden or removing readonly from it.

Since all information is user generated, let's add some hardcoded information to distinguish complete spam from potentially legitimate abuse reports. (This is just a work in progress not the final solution, but I'm done for the night! This is where I left off)
template/emails/report_abuse.html

      <tr>
        <td>Userinfo:</td>
        <td>{userinfo}</td>
      </tr>

actions/reportabuse.php
After 'reason' => $utils->sanitize($_POST['fileabusenote']),

'userinfo' => (isset($_SESSION['user_id']) ? "ID: " .$utils->sanitize($_SESSION['user_id']). " \r\n<br/> " : '') . "UA: " . $utils->sanitize($_SERVER['HTTP_USER_AGENT']) . "\r\n<br/> IP: " . $utils->sanitize($_SERVER['REMOTE_ADDR']),

To-do: some more user info but more importantly implement some sort of unique required key that is generated, stored in session (regardless of logged in status) when they visit reportabuse that expires after the email is sent alongside rate limiting per IP (5/hour probably).

At the moment, I can just spam the owner email by sending a bunch of post requests at reportabuse.php, because there is zero validation or security other than simply checking if the file exists.

It also wouldn't hurt to eventually implement a page in the admin panel to view reported abused files, with the options to download and delete file alongside who reported it and what the reason was, I might do this next. May be easier to stick to emails though, I'll debate it.

[farisc0de]

It also wouldn't hurt to eventually implement a page in the admin panel to view reported abused files, with the options to download and delete file alongside who reported it and what the reason was, I might do this next. May be easier to stick to emails though, I'll debate it.

This actually in my plan with also auto reply to the reporter of the status of issue.