Fix review findings and CI failures on PR #22520

CI fix: InterfaceSerializer.validate() was calling data.pop() without
checking isinstance(data, dict), causing AttributeError when validate()
received an Interface instance via the #18887 custom-field code path.
Added the same isinstance guard already present in VMInterfaceSerializer.

High severity:
- MACAddressSetPrimaryView: remove GET handler (CSRF risk); mutation now
  requires a POST. Table column renders a CSRF-token form for set_primary
  instead of a bare <a> link. View now uses restrict(user, 'view') on the
  MAC queryset.
- Serializers: add dcim.add_macaddress permission check before creating
  MACAddress objects in InterfaceSerializer and VMInterfaceSerializer.
- Wrap multi-step writes in transaction.atomic(); add snapshot() before
  secondary interface saves to produce clean changelog diffs.

Medium severity:
- InterfaceCommonForm.save(): gate all MAC DB writes behind `if commit:`
  to honour the commit=False contract.
- MACAddressActionsColumn.render(): always exclude set_primary from the
  parent action loop (which renders GET links), then inject the POST form
  separately. Avoids temporary mutation of self.actions across branches.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

netbox/dcim/api/serializers_/device_components.py

@@ -1,7 +1,9 @@
 from django.contrib.contenttypes.models import ContentType
+from django.db import transaction
 from django.utils.translation import gettext as _
 from netaddr import EUI, AddrFormatError
 from rest_framework import serializers
+from rest_framework.exceptions import PermissionDenied
 
 from dcim.choices import *
 from dcim.constants import *
@@ -277,9 +279,12 @@ class InterfaceSerializer(
     def validate(self, data):
         # Pop mac_address before model validation — it's a cached_property, not a model field,
         # and passing it to Interface(**attrs) in ValidatedModelSerializer.validate() would raise TypeError.
-        mac_address = data.pop('mac_address', _UNSET)
+        # data may be an Interface instance (not a dict) in some custom field code paths (#18887).
+        mac_address = _UNSET
+        if isinstance(data, dict):
+            mac_address = data.pop('mac_address', _UNSET)
 
-        if not self.nested:
+        if not self.nested and isinstance(data, dict):
             if mac_address not in (_UNSET, None):
                 try:
                     EUI(mac_address, version=48)
@@ -353,33 +358,52 @@ class InterfaceSerializer(
 
     def create(self, validated_data):
         mac_address = validated_data.pop('mac_address', None)
-        instance = super().create(validated_data)
         if mac_address is not None:
-            mac = MACAddress.objects.create(mac_address=mac_address, assigned_object=instance)
-            instance.primary_mac_address = mac
-            instance.save()
-            instance.__dict__.pop('mac_address', None)
+            request = self.context.get('request')
+            if request and not request.user.has_perm('dcim.add_macaddress'):
+                raise PermissionDenied(_('You do not have permission to create MAC addresses.'))
+        with transaction.atomic():
+            instance = super().create(validated_data)
+            if mac_address is not None:
+                mac = MACAddress.objects.create(mac_address=mac_address, assigned_object=instance)
+                instance.primary_mac_address = mac
+                instance.save()
+                instance.__dict__.pop('mac_address', None)
         return instance
 
     def update(self, instance, validated_data):
         mac_address = validated_data.pop('mac_address', _UNSET)
-        instance = super().update(instance, validated_data)
-        if mac_address is _UNSET:
-            pass
-        elif mac_address is None:
-            instance.primary_mac_address = None
-            instance.save()
-            instance.__dict__.pop('mac_address', None)
+
+        # Check permission and locate any existing MAC before any writes.
+        if mac_address not in (_UNSET, None):
+            existing_mac = instance.mac_addresses.filter(mac_address=mac_address).first()
+            if existing_mac is None:
+                request = self.context.get('request')
+                if request and not request.user.has_perm('dcim.add_macaddress'):
+                    raise PermissionDenied(_('You do not have permission to create MAC addresses.'))
         else:
-            # Find an existing MACAddress on this interface with the target value, or create one.
-            # Using find-or-create avoids duplicating a MAC that already exists on this interface.
-            mac = instance.mac_addresses.filter(mac_address=mac_address).first()
-            if mac is None:
-                mac = MACAddress.objects.create(mac_address=mac_address, assigned_object=instance)
-            if instance.primary_mac_address_id != mac.pk:
-                instance.primary_mac_address = mac
-                instance.save()
-            instance.__dict__.pop('mac_address', None)
+            existing_mac = None
+
+        with transaction.atomic():
+            instance = super().update(instance, validated_data)
+            if mac_address is _UNSET:
+                pass
+            elif mac_address is None:
+                if instance.primary_mac_address_id is not None:
+                    instance.snapshot()
+                    instance.primary_mac_address = None
+                    instance.save()
+            else:
+                # Find-or-create: prefer existing MAC on this interface; create only if absent.
+                mac = existing_mac
+                if mac is None:
+                    mac = MACAddress.objects.create(mac_address=mac_address, assigned_object=instance)
+                if instance.primary_mac_address_id != mac.pk:
+                    instance.snapshot()
+                    instance.primary_mac_address = mac
+                    instance.save()
+
+        instance.__dict__.pop('mac_address', None)
         return instance
 
 

netbox/dcim/forms/common.py

@@ -87,7 +87,7 @@ class InterfaceCommonForm(forms.Form):
     def save(self, commit=True):
         instance = super().save(commit=commit)
 
-        if 'mac_address' not in self.changed_data:
+        if not commit or 'mac_address' not in self.changed_data:
             return instance
 
         from dcim.models import MACAddress
@@ -102,12 +102,10 @@ class InterfaceCommonForm(forms.Form):
                 mac.save()
             if instance.primary_mac_address_id != mac.pk:
                 instance.primary_mac_address = mac
-                if commit:
-                    instance.save()
+                instance.save()
         else:
             instance.primary_mac_address = None
-            if commit:
-                instance.save()
+            instance.save()
 
         instance.__dict__.pop('mac_address', None)
         return instance

netbox/dcim/tables/devices.py

@@ -1,4 +1,7 @@
 import django_tables2 as tables
+from django.middleware.csrf import get_token
+from django.urls import reverse
+from django.utils.safestring import mark_safe
 from django.utils.translation import gettext_lazy as _
 from django_tables2.utils import Accessor
 
@@ -1225,15 +1228,35 @@ class MACAddressActionsColumn(columns.ActionsColumn):
     }
 
     def render(self, record, table, **kwargs):
-        # Only offer "Set as primary" for non-primary MACs that are assigned to an interface.
-        if record.is_primary or not record.assigned_object_id:
-            saved = self.actions
-            try:
-                self.actions = {k: v for k, v in saved.items() if k != 'set_primary'}
-                return super().render(record, table, **kwargs)
-            finally:
-                self.actions = saved
-        return super().render(record, table, **kwargs)
+        # Always exclude set_primary from the parent's action loop (which renders GET links).
+        # We inject a CSRF-protected POST form for set_primary in the dropdown below.
+        show_set_primary = not record.is_primary and record.assigned_object_id
+        original_actions = self.actions
+        self.actions = {k: v for k, v in original_actions.items() if k != 'set_primary'}
+        try:
+            html = super().render(record, table, **kwargs)
+        finally:
+            self.actions = original_actions
+
+        if show_set_primary and html:
+            request = getattr(table, 'context', {}).get('request')
+            if request:
+                url = reverse('dcim:macaddress_set_primary', kwargs={'pk': record.pk})
+                form_li = (
+                    f'<li>'
+                    f'<form method="post" action="{url}">'
+                    f'<input type="hidden" name="csrfmiddlewaretoken" value="{get_token(request)}">'
+                    f'<button type="submit" class="dropdown-item">'
+                    f'<i class="mdi mdi-star-outline"></i> {_("Set as primary")}'
+                    f'</button>'
+                    f'</form>'
+                    f'</li>'
+                )
+                html_str = str(html)
+                if '</ul>' in html_str:
+                    html = mark_safe(html_str.replace('</ul>', form_li + '</ul>', 1))
+
+        return html
 
 
 class MACAddressTable(PrimaryModelTable):

netbox/dcim/tests/test_views.py

@@ -4405,7 +4405,7 @@ class MACAddressTestCase(ViewTestCases.PrimaryObjectViewTestCase):
         self.assertIsNone(interface.primary_mac_address)
 
         url = reverse('dcim:macaddress_set_primary', kwargs={'pk': mac.pk})
-        response = self.client.get(url)
+        response = self.client.post(url)
 
         self.assertHttpStatus(response, 302)
         self.assertEqual(response['Location'], interface.get_absolute_url())
@@ -4425,7 +4425,7 @@ class MACAddressTestCase(ViewTestCases.PrimaryObjectViewTestCase):
         interface.save()
 
         url = reverse('dcim:macaddress_set_primary', kwargs={'pk': mac.pk})
-        response = self.client.get(url)
+        response = self.client.post(url)
 
         self.assertHttpStatus(response, 302)
         self.assertEqual(response['Location'], interface.get_absolute_url())
@@ -4441,7 +4441,7 @@ class MACAddressTestCase(ViewTestCases.PrimaryObjectViewTestCase):
 
         mac = MACAddress.objects.first()
         url = reverse('dcim:macaddress_set_primary', kwargs={'pk': mac.pk})
-        response = self.client.get(url)
+        response = self.client.post(url)
 
         self.assertHttpStatus(response, 302)
         self.assertEqual(response['Location'], mac.get_absolute_url())

netbox/dcim/views.py

@@ -5034,18 +5034,12 @@ class MACAddressDeleteView(generic.ObjectDeleteView):
 class MACAddressSetPrimaryView(View):
     queryset = MACAddress.objects.all()
 
-    def get(self, request, pk):
-        return self._handle(request, pk)
-
     def post(self, request, pk):
-        return self._handle(request, pk)
-
-    def _handle(self, request, pk):
         if not request.user.is_authenticated:
             from django.contrib.auth.views import redirect_to_login
             return redirect_to_login(request.get_full_path())
 
-        mac = get_object_or_404(self.queryset, pk=pk)
+        mac = get_object_or_404(self.queryset.restrict(request.user, 'view'), pk=pk)
         assigned_object = mac.assigned_object
 
         if assigned_object is None:

netbox/virtualization/api/serializers_/virtualmachines.py

@@ -1,7 +1,9 @@
+from django.db import transaction
 from django.utils.translation import gettext as _
 from drf_spectacular.utils import extend_schema_field
 from netaddr import EUI, AddrFormatError
 from rest_framework import serializers
+from rest_framework.exceptions import PermissionDenied
 
 from dcim.api.serializers_.devices import DeviceSerializer, MACAddressSerializer
 from dcim.api.serializers_.platforms import PlatformSerializer
@@ -192,33 +194,52 @@ class VMInterfaceSerializer(OwnerMixin, NetBoxModelSerializer):
 
     def create(self, validated_data):
         mac_address = validated_data.pop('mac_address', None)
-        instance = super().create(validated_data)
         if mac_address is not None:
-            mac = MACAddress.objects.create(mac_address=mac_address, assigned_object=instance)
-            instance.primary_mac_address = mac
-            instance.save()
-            instance.__dict__.pop('mac_address', None)
+            request = self.context.get('request')
+            if request and not request.user.has_perm('dcim.add_macaddress'):
+                raise PermissionDenied(_('You do not have permission to create MAC addresses.'))
+        with transaction.atomic():
+            instance = super().create(validated_data)
+            if mac_address is not None:
+                mac = MACAddress.objects.create(mac_address=mac_address, assigned_object=instance)
+                instance.primary_mac_address = mac
+                instance.save()
+                instance.__dict__.pop('mac_address', None)
         return instance
 
     def update(self, instance, validated_data):
         mac_address = validated_data.pop('mac_address', _UNSET)
-        instance = super().update(instance, validated_data)
-        if mac_address is _UNSET:
-            pass
-        elif mac_address is None:
-            instance.primary_mac_address = None
-            instance.save()
-            instance.__dict__.pop('mac_address', None)
+
+        # Check permission and locate any existing MAC before any writes.
+        if mac_address not in (_UNSET, None):
+            existing_mac = instance.mac_addresses.filter(mac_address=mac_address).first()
+            if existing_mac is None:
+                request = self.context.get('request')
+                if request and not request.user.has_perm('dcim.add_macaddress'):
+                    raise PermissionDenied(_('You do not have permission to create MAC addresses.'))
         else:
-            # Find an existing MACAddress on this interface with the target value, or create one.
-            # Using find-or-create avoids duplicating a MAC that already exists on this interface.
-            mac = instance.mac_addresses.filter(mac_address=mac_address).first()
-            if mac is None:
-                mac = MACAddress.objects.create(mac_address=mac_address, assigned_object=instance)
-            if instance.primary_mac_address_id != mac.pk:
-                instance.primary_mac_address = mac
-                instance.save()
-            instance.__dict__.pop('mac_address', None)
+            existing_mac = None
+
+        with transaction.atomic():
+            instance = super().update(instance, validated_data)
+            if mac_address is _UNSET:
+                pass
+            elif mac_address is None:
+                if instance.primary_mac_address_id is not None:
+                    instance.snapshot()
+                    instance.primary_mac_address = None
+                    instance.save()
+            else:
+                # Find-or-create: prefer existing MAC on this interface; create only if absent.
+                mac = existing_mac
+                if mac is None:
+                    mac = MACAddress.objects.create(mac_address=mac_address, assigned_object=instance)
+                if instance.primary_mac_address_id != mac.pk:
+                    instance.snapshot()
+                    instance.primary_mac_address = mac
+                    instance.save()
+
+        instance.__dict__.pop('mac_address', None)
         return instance