Fixes #20236: Improve file naming and upload handling

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 @@ def delete(self, *args, **kwargs):
 
     @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 @@ def test_virtualmachine_site_context(self):
         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 @@ def test_multiple_tags_return_distinct_objects(self):
         """
         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 @@ def test_multiple_tags_return_distinct_objects(self):
         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
-        context assigned to two tags and ensures objects related by those same two tags result in only a single
+        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 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 @@ def test_expected_output(self):
 
         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_'))