Fixes #20236: Improve file naming and upload handling (#20315)

netbox/extras/models/models.py

@@ -1,6 +1,6 @@
 import json
-import os
 import urllib.parse
+from pathlib import Path
 
 from django.conf import settings
 from django.contrib.contenttypes.fields import GenericForeignKey, GenericRelation
@@ -728,7 +728,9 @@ class ImageAttachment(ChangeLoggedModel):
 
     @property
     def filename(self):
-        return os.path.basename(self.image.name).split('_', 2)[2]
+        base_name = Path(self.image.name).name
+        prefix = f"{self.object_type.model}_{self.object_id}_"
+        return base_name.removeprefix(prefix)
 
     @property
     def html_tag(self):

netbox/extras/tests/test_models.py

@@ -1,17 +1,95 @@
 import tempfile
 from pathlib import Path
 
+from django.contrib.contenttypes.models import ContentType
+from django.core.files.uploadedfile import SimpleUploadedFile
 from django.forms import ValidationError
 from django.test import tag, TestCase
 
 from core.models import DataSource, ObjectType
 from dcim.models import Device, DeviceRole, DeviceType, Location, Manufacturer, Platform, Region, Site, SiteGroup
-from extras.models import ConfigContext, ConfigContextProfile, ConfigTemplate, Tag
+from extras.models import ConfigContext, ConfigContextProfile, ConfigTemplate, ImageAttachment, Tag
 from tenancy.models import Tenant, TenantGroup
 from utilities.exceptions import AbortRequest
 from virtualization.models import Cluster, ClusterGroup, ClusterType, VirtualMachine
 
 
+class ImageAttachmentTests(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        cls.ct_rack = ContentType.objects.get(app_label='dcim', model='rack')
+        cls.image_content = b''
+
+    def _stub_image_attachment(self, object_id, image_filename, name=None):
+        """
+        Creates an instance of ImageAttachment with the provided object_id and image_name.
+
+        This method prepares a stubbed image attachment to test functionalities that
+        require an ImageAttachment object.
+        The function initializes the attachment with a specified file name and
+        pre-defined image content.
+        """
+        ia = ImageAttachment(
+            object_type=self.ct_rack,
+            object_id=object_id,
+            name=name,
+            image=SimpleUploadedFile(
+                name=image_filename,
+                content=self.image_content,
+                content_type='image/jpeg',
+            ),
+        )
+        return ia
+
+    def test_filename_strips_expected_prefix(self):
+        """
+        Tests that the filename of the image attachment is stripped of the expected
+        prefix.
+        """
+        ia = self._stub_image_attachment(12, 'image-attachments/rack_12_My_File.png')
+        self.assertEqual(ia.filename, 'My_File.png')
+
+    def test_filename_legacy_nested_path_returns_basename(self):
+        """
+        Tests if the filename of a legacy-nested path correctly returns only the basename.
+        """
+        # e.g. "image-attachments/rack_12_5/31/23.jpg" -> "23.jpg"
+        ia = self._stub_image_attachment(12, 'image-attachments/rack_12_5/31/23.jpg')
+        self.assertEqual(ia.filename, '23.jpg')
+
+    def test_filename_no_prefix_returns_basename(self):
+        """
+        Tests that the filename property correctly returns the basename for an image
+        attachment that has no leading prefix in its path.
+        """
+        ia = self._stub_image_attachment(42, 'image-attachments/just_name.webp')
+        self.assertEqual(ia.filename, 'just_name.webp')
+
+    def test_mismatched_prefix_is_not_stripped(self):
+        """
+        Tests that a mismatched prefix in the filename is not stripped.
+        """
+        # Prefix does not match object_id -> leave as-is (basename only)
+        ia = self._stub_image_attachment(12, 'image-attachments/rack_13_other.png')
+        self.assertEqual('rack_13_other.png', ia.filename)
+
+    def test_str_uses_name_when_present(self):
+        """
+        Tests that the `str` representation of the object uses the
+        `name` attribute when provided.
+        """
+        ia = self._stub_image_attachment(12, 'image-attachments/rack_12_file.png', name='Human title')
+        self.assertEqual('Human title', str(ia))
+
+    def test_str_falls_back_to_filename(self):
+        """
+        Tests that the `str` representation of the object falls back to
+        the filename if the name attribute is not set.
+        """
+        ia = self._stub_image_attachment(12, 'image-attachments/rack_12_file.png', name='')
+        self.assertEqual('file.png', str(ia))
+
+
 class TagTest(TestCase):
 
     def test_default_ordering_weight_then_name_is_set(self):
@@ -445,7 +523,7 @@ class ConfigContextTest(TestCase):
         vm1 = VirtualMachine.objects.create(name="VM 1", site=site, role=vm_role)
         vm2 = VirtualMachine.objects.create(name="VM 2", cluster=cluster, role=vm_role)
 
-        # Check that their individually-rendered config contexts are identical
+        # Check that their individually rendered config contexts are identical
         self.assertEqual(
             vm1.get_config_context(),
             vm2.get_config_context()
@@ -462,7 +540,7 @@ class ConfigContextTest(TestCase):
         """
         Tagged items use a generic relationship, which results in duplicate rows being returned when queried.
         This is combated by appending distinct() to the config context querysets. This test creates a config
-        context assigned to two tags and ensures objects related by those same two tags result in only a single
+        context assigned to two tags and ensures objects related to those same two tags result in only a single
         config context record being returned.
 
         See https://github.com/netbox-community/netbox/issues/5314
@@ -495,14 +573,14 @@ class ConfigContextTest(TestCase):
         self.assertEqual(ConfigContext.objects.get_for_object(device).count(), 1)
         self.assertEqual(device.get_config_context(), annotated_queryset[0].get_config_context())
 
-    def test_multiple_tags_return_distinct_objects_with_seperate_config_contexts(self):
+    def test_multiple_tags_return_distinct_objects_with_separate_config_contexts(self):
         """
         Tagged items use a generic relationship, which results in duplicate rows being returned when queried.
-        This is combatted by by appending distinct() to the config context querysets. This test creates a config
+        This is combated by appending distinct() to the config context querysets. This test creates a config
-        context assigned to two tags and ensures objects related by those same two tags result in only a single
+        context assigned to two tags and ensures objects related to those same two tags result in only a single
         config context record being returned.
 
-        This test case is seperate from the above in that it deals with multiple config context objects in play.
+        This test case is separate from the above in that it deals with multiple config context objects in play.
 
         See https://github.com/netbox-community/netbox/issues/5387
         """

netbox/extras/tests/test_utils.py

@@ -1,7 +1,10 @@
+from types import SimpleNamespace
+
+from django.contrib.contenttypes.models import ContentType
 from django.test import TestCase
 
 from extras.models import ExportTemplate
-from extras.utils import filename_from_model
+from extras.utils import filename_from_model, image_upload
 from tenancy.models import ContactGroup, TenantGroup
 from wireless.models import WirelessLANGroup
 
@@ -17,3 +20,141 @@ class FilenameFromModelTests(TestCase):
 
         for model, expected in cases:
             self.assertEqual(filename_from_model(model), expected)
+
+
+class ImageUploadTests(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        # We only need a ContentType with model="rack" for the prefix;
+        # this doesn't require creating a Rack object.
+        cls.ct_rack = ContentType.objects.get(app_label='dcim', model='rack')
+
+    def _stub_instance(self, object_id=12, name=None):
+        """
+        Creates a minimal stub for use with the `image_upload()` function.
+
+        This method generates an instance of `SimpleNamespace` containing a set
+        of attributes required to simulate the expected input for the
+        `image_upload()` method.
+        It is designed to simplify testing or processing by providing a
+        lightweight representation of an object.
+        """
+        return SimpleNamespace(object_type=self.ct_rack, object_id=object_id, name=name)
+
+    def _second_segment(self, path: str):
+        """
+        Extracts and returns the portion of the input string after the
+        first '/' character.
+        """
+        return path.split('/', 1)[1]
+
+    def test_windows_fake_path_and_extension_lowercased(self):
+        """
+        Tests handling of a Windows file path with a fake directory and extension.
+        """
+        inst = self._stub_instance(name=None)
+        path = image_upload(inst, r'C:\fake_path\MyPhoto.JPG')
+        # Base directory and single-level path
+        seg2 = self._second_segment(path)
+        self.assertTrue(path.startswith('image-attachments/rack_12_'))
+        self.assertNotIn('/', seg2, 'should not create nested directories')
+        # Extension from the uploaded file, lowercased
+        self.assertTrue(seg2.endswith('.jpg'))
+
+    def test_name_with_slashes_is_flattened_no_subdirectories(self):
+        """
+        Tests that a name with slashes is flattened and does not
+        create subdirectories.
+        """
+        inst = self._stub_instance(name='5/31/23')
+        path = image_upload(inst, 'image.png')
+        seg2 = self._second_segment(path)
+        self.assertTrue(seg2.startswith('rack_12_'))
+        self.assertNotIn('/', seg2)
+        self.assertNotIn('\\', seg2)
+        self.assertTrue(seg2.endswith('.png'))
+
+    def test_name_with_backslashes_is_flattened_no_subdirectories(self):
+        """
+        Tests that a name including backslashes is correctly flattened
+        into a single directory name without creating subdirectories.
+        """
+        inst = self._stub_instance(name=r'5\31\23')
+        path = image_upload(inst, 'image_name.png')
+
+        seg2 = self._second_segment(path)
+        self.assertTrue(seg2.startswith('rack_12_'))
+        self.assertNotIn('/', seg2)
+        self.assertNotIn('\\', seg2)
+        self.assertTrue(seg2.endswith('.png'))
+
+    def test_prefix_format_is_as_expected(self):
+        """
+        Tests the output path format generated by the `image_upload` function.
+        """
+        inst = self._stub_instance(object_id=99, name='label')
+        path = image_upload(inst, 'a.webp')
+        # The second segment must begin with "rack_99_"
+        seg2 = self._second_segment(path)
+        self.assertTrue(seg2.startswith('rack_99_'))
+        self.assertTrue(seg2.endswith('.webp'))
+
+    def test_unsupported_file_extension(self):
+        """
+        Test that when the file extension is not allowed, the extension
+        is omitted.
+        """
+        inst = self._stub_instance(name='test')
+        path = image_upload(inst, 'document.txt')
+
+        seg2 = self._second_segment(path)
+        self.assertTrue(seg2.startswith('rack_12_test'))
+        self.assertFalse(seg2.endswith('.txt'))
+        # When not allowed, no extension should be appended
+        self.assertNotRegex(seg2, r'\.txt$')
+
+    def test_instance_name_with_whitespace_and_special_chars(self):
+        """
+        Test that an instance name with leading/trailing whitespace and
+        special characters is sanitized properly.
+        """
+        # Suppose the instance name has surrounding whitespace and
+        # extra slashes.
+        inst = self._stub_instance(name='  my/complex\\name  ')
+        path = image_upload(inst, 'irrelevant.png')
+
+        # The output should be flattened and sanitized.
+        # We expect the name to be transformed into a valid filename without
+        # path separators.
+        seg2 = self._second_segment(path)
+        self.assertNotIn(' ', seg2)
+        self.assertNotIn('/', seg2)
+        self.assertNotIn('\\', seg2)
+        self.assertTrue(seg2.endswith('.png'))
+
+    def test_separator_variants_with_subTest(self):
+        """
+        Tests that both forward slash and backslash in file paths are
+        handled consistently by the `image_upload` function and
+        processed into a sanitized uniform format.
+        """
+        for name in ['2025/09/12', r'2025\09\12']:
+            with self.subTest(name=name):
+                inst = self._stub_instance(name=name)
+                path = image_upload(inst, 'x.jpeg')
+                seg2 = self._second_segment(path)
+                self.assertTrue(seg2.startswith('rack_12_'))
+                self.assertNotIn('/', seg2)
+                self.assertNotIn('\\', seg2)
+                self.assertTrue(seg2.endswith('.jpeg') or seg2.endswith('.jpg'))
+
+    def test_fallback_on_suspicious_file_operation(self):
+        """
+        Test that when default_storage.get_valid_name() raises a
+        SuspiciousFileOperation, the fallback default is used.
+        """
+        inst = self._stub_instance(name=' ')
+        path = image_upload(inst, 'sample.png')
+        # Expect the fallback name 'unnamed' to be used.
+        self.assertIn('unnamed', path)
+        self.assertTrue(path.startswith('image-attachments/rack_12_'))

netbox/extras/utils.py

@@ -1,15 +1,20 @@
 import importlib
+from pathlib import Path
 
-from django.core.exceptions import ImproperlyConfigured
+from django.core.exceptions import ImproperlyConfigured, SuspiciousFileOperation
+from django.core.files.storage import default_storage
+from django.core.files.utils import validate_file_name
 from django.db import models
 from django.db.models import Q
 from taggit.managers import _TaggableManager
 
 from netbox.context import current_request
+
 from .validators import CustomValidator
 
 __all__ = (
     'SharedObjectViewMixin',
+    'filename_from_model',
     'image_upload',
     'is_report',
     'is_script',
@@ -35,13 +40,13 @@ class SharedObjectViewMixin:
 
 
 def filename_from_model(model: models.Model) -> str:
-    """Standardises how we generate filenames from model class for exports"""
+    """Standardizes how we generate filenames from model class for exports"""
     base = model._meta.verbose_name_plural.lower().replace(' ', '_')
     return f'netbox_{base}'
 
 
 def filename_from_object(context: dict) -> str:
-    """Standardises how we generate filenames from model class for exports"""
+    """Standardizes how we generate filenames from model class for exports"""
     if 'device' in context:
         base = f"{context['device'].name or 'config'}"
     elif 'virtualmachine' in context:
@@ -64,17 +69,42 @@ def is_taggable(obj):
 def image_upload(instance, filename):
     """
     Return a path for uploading image attachments.
+
+    - Normalizes browser paths (e.g., C:\\fake_path\\photo.jpg)
+    - Uses the instance.name if provided (sanitized to a *basename*, no ext)
+    - Prefixes with a machine-friendly identifier
+
+    Note: Relies on Django's default_storage utility.
     """
-    path = 'image-attachments/'
+    upload_dir = 'image-attachments'
+    default_filename = 'unnamed'
+    allowed_img_extensions = ('bmp', 'gif', 'jpeg', 'jpg', 'png', 'webp')
 
-    # Rename the file to the provided name, if any. Attempt to preserve the file extension.
+    # Normalize Windows paths and create a Path object.
-    extension = filename.rsplit('.')[-1].lower()
+    normalized_filename = str(filename).replace('\\', '/')
-    if instance.name and extension in ['bmp', 'gif', 'jpeg', 'jpg', 'png', 'webp']:
+    file_path = Path(normalized_filename)
-        filename = '.'.join([instance.name, extension])
-    elif instance.name:
-        filename = instance.name
 
-    return '{}{}_{}_{}'.format(path, instance.object_type.name, instance.object_id, filename)
+    # Extract the extension from the uploaded file.
+    ext = file_path.suffix.lower().lstrip('.')
+
+    # Use the instance-provided name if available; otherwise use the file stem.
+    # Rely on Django's get_valid_filename to perform sanitization.
+    stem = (instance.name or file_path.stem).strip()
+    try:
+        safe_stem = default_storage.get_valid_name(stem)
+    except SuspiciousFileOperation:
+        safe_stem = default_filename
+
+    # Append the uploaded extension only if it's an allowed image type
+    final_name = f"{safe_stem}.{ext}" if ext in allowed_img_extensions else safe_stem
+
+    # Create a machine-friendly prefix from the instance
+    prefix = f"{instance.object_type.model}_{instance.object_id}"
+    name_with_path = f"{upload_dir}/{prefix}_{final_name}"
+
+    # Validate the generated relative path (blocks absolute/traversal)
+    validate_file_name(name_with_path, allow_relative_path=True)
+    return name_with_path
 
 
 def is_script(obj):
@@ -107,7 +137,7 @@ def run_validators(instance, validators):
     request = current_request.get()
     for validator in validators:
 
-        # Loading a validator class by dotted path
+        # Loading a validator class by a dotted path
         if type(validator) is str:
             module, cls = validator.rsplit('.', 1)
             validator = getattr(importlib.import_module(module), cls)()