[BUG] Uploading a product image with an existing filename overwrites the image o

Started by wyder, Today at 18:12:37 PM

Previous topic - Next topic

wyder

Hello,

I have found a reproducible problem with product image uploads in VirtueMart 4.6.8.

### Environment

* VirtueMart: 4.6.8 build 11258
* Joomla: 5.4.7
* PHP: 8.4.22

### Problem description

When a new product image has the same filename as an image that already exists in the VirtueMart product image directory, VirtueMart overwrites the existing physical file.

This means that uploading an image to one product can unexpectedly replace an image already used by another product.

This is particularly dangerous in stores with many products, because administrators cannot realistically ensure that every uploaded image has a globally unique filename.

### Steps to reproduce

1. Create or edit Product A.
2. Upload an image named: 1.jpg
3. Save Product A.
4. Create or edit Product B.
5. Upload a different image which is also named: 1.jpg
6. Save Product B.
7. Return to Product A or view it on the frontend.

### Actual result

The physical file `1.jpg` is overwritten by the image uploaded to Product B.

As a result, Product A starts displaying the image uploaded for Product B.

The problem occurs without any warning to the administrator.

### Expected result

When `$overwrite` is set to `false` and a file with the same name already exists, VirtueMart should generate and use a unique filename, for example:

1.jpg
17.jpg
173.jpg

The existing physical file should not be overwritten.

### Code analysis

The problem appears to be located in:

administrator/components/com_virtuemart/helpers/vmuploader.php


in the method:
static function checkUploadFile($uploadPath, &$media, $overwrite, &$isimage)


The current code correctly checks whether a file with the same filename already exists:
if(!$overwrite){
    $i = 0;

    while (file_exists($uploadPath.$mediaPure.'.'.$mediaExtension) and $i<20) {
        $mediaPure = $mediaPure.rand(1,9);
        $i++;
    }

    if($i>=20){
        vmError('Could not upload file, would overwrite existing '.$media['name']);
        return false;
    }
}


The loop modifies `$mediaPure` and generates a new filename base.

For example:
1


may be changed to:
17


However, the generated filename is never assigned back to:
$media['name']


The upload is then performed using the original filename:

$uploaded = JFile::upload(
    $media['tmp_name'],
    $uploadPath.$media['name'],
    false,
    $trusted
);


At this point, `$media['name']` still contains:
1.jpg


Therefore, the existing file is overwritten even though the collision detection code generated a different value in `$mediaPure`.

### Suggested fix

After the filename collision check and before calling `JFile::upload()`, the generated filename should be assigned back to `$media['name']`:

$media['name'] = $mediaPure.'.'.$mediaExtension;


The corrected code would be:

if(!$overwrite){
    $i = 0;

    while (file_exists($uploadPath.$mediaPure.'.'.$mediaExtension) and $i<20) {
        $mediaPure = $mediaPure.rand(1,9);
        $i++;
    }

    if($i>=20){
        vmError('Could not upload file, would overwrite existing '.$media['name']);
        return false;
    }
}

$media['name'] = $mediaPure.'.'.$mediaExtension;

$uploaded = JFile::upload(
    $media['tmp_name'],
    $uploadPath.$media['name'],
    false,
    $trusted
);

Because `$media` is passed by reference:

static function checkUploadFile($uploadPath, &$media, $overwrite, &$isimage)


the corrected filename will also be available later in `uploadFile()`:
$obj->file_name = $media['name'];


and should therefore be saved correctly in the VirtueMart media record.

### Additional information

I checked both:

* the `vmuploader.php` file currently installed on the website,
* the `vmuploader.php` file from the original VirtueMart 4.6.8 installation package.

Both files contain the same code.

Therefore, the issue does not appear to be caused by a modified, incomplete or corrupted installation.

Adding the following line before `JFile::upload()` prevents the existing file from being overwritten:


$media['name'] = $mediaPure.'.'.$mediaExtension;


Could you please confirm whether this is a bug and include the fix in the next VirtueMart release?

Thank you.