[fix] Avoid mutating device name in-place inside _get_common_name()

Author: mn-ram

openwisp_controller/config/base/vpn.py

@@ -892,12 +892,20 @@ def _get_common_name(self):
         """
         d = self.config.device
         end = 63 - len(d.mac_address)
-        d.name = d.name[:end]
+        # Use a local variable to avoid mutating the device instance in-place.
+        # Mutating d.name would corrupt the device object shared with the caller
+        # (Django caches FK targets), causing the registration response hostname
+        # and any subsequent device.save() to silently persist the truncated name.
+        name = d.name[:end]
         unique_slug = shortuuid.ShortUUID().random(length=8)
         cn_format = app_settings.COMMON_NAME_FORMAT
-        if cn_format == "{mac_address}-{name}" and d.name == d.mac_address:
+        if cn_format == "{mac_address}-{name}" and name == d.mac_address:
             cn_format = "{mac_address}"
-        common_name = cn_format.format(**d.__dict__)[:55]
+        # Build the format context explicitly so custom COMMON_NAME_FORMAT strings
+        # that reference other device attributes (e.g. {hardware_id}) still work,
+        # while the truncated name is used without touching the device object.
+        context = {**d.__dict__, "name": name}
+        common_name = cn_format.format(**context)[:55]
         common_name = f"{common_name}-{unique_slug}"
         return common_name
 

openwisp_controller/config/tests/test_controller.py

@@ -1289,6 +1289,51 @@ def test_register_template_tags(self):
         self.assertEqual(d.config.templates.filter(pk=t_shared.pk).count(), 1)
         self.assertEqual(d.config.templates.filter(pk=t2.pk).count(), 0)
 
+    def test_register_hostname_not_truncated_by_vpn_cert_provisioning(self):
+        """
+        When a new device with a long name registers and a VPN template with
+        auto_cert=True is applied via tags, _get_common_name() must not mutate
+        the in-memory device object.  The `hostname` field in the registration
+        response and the name stored in the database must both equal the full
+        original name, not a cert-length-truncated version.
+        """
+        # Name longer than the truncation threshold used in _get_common_name()
+        # (63 - len("00:11:22:33:44:55") = 46 chars).
+        long_name = "a-very-long-device-hostname-that-exceeds-46-chars"
+        self.assertGreater(len(long_name), 46)
+
+        org = self._create_org(name="org1")
+        vpn = self._create_vpn(organization=org)
+        # VPN template tagged "openvpn" with auto_cert so _get_common_name() runs.
+        t = self._create_template(
+            name="vpn-tpl",
+            organization=org,
+            type="vpn",
+            auto_cert=True,
+            vpn=vpn,
+        )
+        t.tags.add("openvpn")
+
+        response = self.client.post(
+            self.register_url,
+            {
+                "secret": TEST_ORG_SHARED_SECRET,
+                "name": long_name,
+                "mac_address": TEST_MACADDR,
+                "backend": "netjsonconfig.OpenWrt",
+                "tags": "openvpn",
+            },
+        )
+        self.assertEqual(response.status_code, 201)
+
+        # The `hostname` line in the response must be the full original name.
+        response_text = response.content.decode()
+        self.assertIn(f"hostname: {long_name}", response_text)
+
+        # The database record must also store the full original name.
+        device = Device.objects.get(mac_address=TEST_MACADDR)
+        self.assertEqual(device.name, long_name)
+
     @capture_any_output()
     def test_register_400(self):
         self._get_org()

openwisp_controller/config/tests/test_vpn.py

@@ -414,13 +414,60 @@ def test_auto_create_cert_with_long_device_name(self):
         client.full_clean()
         client.save()
         # The last 9 characters gets truncated and replaced with unique id
-        self.assertIn(
-            "{mac_address}-{name}".format(**d.__dict__)[:-9], client._get_common_name()
-        )
+        mac = d.mac_address
+        self.assertIn(f"{mac}-{device_name}"[:-9], client._get_common_name())
         self.assertEqual(len(client._get_common_name()), 64)
         cert = Cert.objects.filter(organization=org, name=device_name)
         self.assertEqual(cert.count(), 1)
         self.assertEqual(cert.first().common_name[:-9], client._get_common_name()[:-9])
+        # The device name must NOT be mutated by _get_common_name()
+        self.assertEqual(d.name, device_name)
+
+    def test_get_common_name_does_not_mutate_device_name(self):
+        """
+        _get_common_name() must not modify the device.name attribute.
+        Django caches FK targets, so mutating the device inside _get_common_name()
+        would corrupt the caller's reference to the same object and could cause
+        a truncated name to be saved to the database on a subsequent device.save().
+        """
+        # Use a name longer than the truncation threshold (63 - len(mac) = 46 chars
+        # for a standard 17-char MAC address).
+        long_name = "a" * 50
+        org = self._create_org(name="org1")
+        vpn = self._create_vpn(organization=org)
+        d = self._create_device(organization=org, name=long_name)
+        c = self._create_config(device=d)
+        client = VpnClient(vpn=vpn, config=c, auto_cert=True)
+
+        client._get_common_name()
+
+        # The in-memory device name must remain unchanged after _get_common_name().
+        self.assertEqual(d.name, long_name)
+        # The database record must also be unaffected.
+        self.assertEqual(
+            d.__class__.objects.values_list("name", flat=True).get(pk=d.pk),
+            long_name,
+        )
+
+    def test_get_common_name_does_not_mutate_device_name_short_name(self):
+        """
+        For device names below the truncation threshold _get_common_name()
+        must still not touch the device object even though the slice is a no-op.
+        """
+        short_name = "router-01"
+        org = self._create_org(name="org1")
+        vpn = self._create_vpn(organization=org)
+        d = self._create_device(organization=org, name=short_name)
+        c = self._create_config(device=d)
+        client = VpnClient(vpn=vpn, config=c, auto_cert=True)
+
+        # Capture the actual string object stored on the device before the call.
+        name_before = d.__dict__["name"]
+        client._get_common_name()
+
+        self.assertEqual(d.name, short_name)
+        # The same string object must still be on the device — not a fresh slice.
+        self.assertIs(d.__dict__["name"], name_before)
 
     @mock.patch.object(Vpn, "dhparam", side_effect=SoftTimeLimitExceeded)
     def test_update_vpn_dh_timeout(self, dhparam):