diff --git a/netbox/extras/models/models.py b/netbox/extras/models/models.py index be4c44d63..7361d087d 100644 --- a/netbox/extras/models/models.py +++ b/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): diff --git a/netbox/extras/tests/test_models.py b/netbox/extras/tests/test_models.py index 341920a81..905ceff88 100644 --- a/netbox/extras/tests/test_models.py +++ b/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 - 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 """ diff --git a/netbox/extras/tests/test_utils.py b/netbox/extras/tests/test_utils.py index b851acab8..ec0102887 100644 --- a/netbox/extras/tests/test_utils.py +++ b/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_')) diff --git a/netbox/extras/utils.py b/netbox/extras/utils.py index c9f554d22..761f4affb 100644 --- a/netbox/extras/utils.py +++ b/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. - extension = filename.rsplit('.')[-1].lower() - if instance.name and extension in ['bmp', 'gif', 'jpeg', 'jpg', 'png', 'webp']: - filename = '.'.join([instance.name, extension]) - elif instance.name: - filename = instance.name + # Normalize Windows paths and create a Path object. + normalized_filename = str(filename).replace('\\', '/') + file_path = Path(normalized_filename) - 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)()