[fix] _get_common_name() must not mutate the device name in-place

[mn-ram]

"""
[fix] Prevent silent device name corruption in _get_common_name()

Problem

AbstractVpnClient._get_common_name() (openwisp_controller/config/base/vpn.py) was truncating the device name for cert CN generation by writing directly back onto the model instance:

d.name = d.name[:end] # unintended in-place mutation

This is a silent data corruption bug. Because Django caches FK targets on model instances, self.config.device inside _get_common_name() is the exact same Python object as the device variable held
by DeviceRegisterView.post(). The mutation therefore escaped the method boundary, with two concrete downstream effects:

• Registration response corrupted: The hostname field returned to the device contained the truncated name, not the original.
• Permanent DB corruption on re-registration: On the device's next registration, it sent back the truncated hostname. _update_device_name() then saved this shortened string to the
  database—overwriting the operator-configured name permanently. No exception was raised, no log entry was written.

Trigger conditions (both required):

• Device name longer than 63 − len(mac_address) characters (> 46 chars for a standard 17-char MAC address).
• A VPN template with auto_cert=True applied at registration time via tags or a device group.

Why the existing test didn't catch this:

• test_auto_create_cert_with_long_device_name used a 46-character name—exactly at the truncation boundary.
• d.name[:46] was a value-preserving no-op. The mutation happened but produced no observable change, masking the bug entirely.

---

Fix

• Use a local variable for the truncated name instead of writing back to the device instance.
• Preserve support for custom COMMON_NAME_FORMAT strings that may reference other device attributes (e.g. {hardware_id}):
  • name = d.name[:end] # local variable — device never touched
  • context = {**d.__dict__, "name": name} # all device fields still accessible
  • common_name = cn_format.format(**context)[:55]
• Minimal, targeted change: 3 lines in production code, zero logic changes, zero new dependencies, fully backward-compatible.

---

Edge Cases Handled

• Name shorter than threshold (e.g. router-01): value-preserving no-op; device object untouched; confirmed via object-identity assertion in tests
• Name exactly at boundary (46 chars): no change in value or object; existing test asserts d.name is unchanged
• Name longer than threshold: name is truncated correctly; d.name stays at original full length
• Custom COMMON_NAME_FORMAT using non-name fields: context spreads all device attributes; any {hardware_id}, {os}, etc. references continue to resolve
• MAC address equals device name (MAC-only format fallback): comparison uses the local variable; identical semantic to before

---

Tests

openwisp_controller/config/tests/test_vpn.py

• test_get_common_name_does_not_mutate_device_name (new): Primary regression guard for 50-char name (above threshold)
• test_get_common_name_does_not_mutate_device_name_short_name (new): Below-threshold case; guards against silent no-op assignment
• test_auto_create_cert_with_long_device_name (updated): Existing test hardened; asserts d.name == device_name; format string explicit

openwisp_controller/config/tests/test_controller.py

• test_register_hostname_not_truncated_by_vpn_cert_provisioning (new): End-to-end integration test; registers a new device with long name and VPN auto-cert tag; asserts response hostname and DB record match original full name

---

Real-World Impact

• Enterprise deployments with descriptive hostnames would experience silent name corruption after re-registration of devices with VPN auto-cert templates
• Corrupted names propagate to monitoring dashboards, notifications, and audit logs, making device identification unreliable
• Fix eliminates the corruption with no behavioral change to certificate generation or other system functionality

---

Notes for Reviewers

• Commit is one logical change; no surrounding code refactoring
• PR touches only the faulty method and its test coverage; no unrelated files modified
• COMMON_NAME_FORMAT backward compatibility preserved ({**d.dict, "name": name} pattern)
  """

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8170c592-110f-4309-83ad-f7fd7d54b50a

📥 Commits

Reviewing files that changed from the base of the PR and between e010474 and be3f41a.

📒 Files selected for processing (3)

• openwisp_controller/config/base/vpn.py
• openwisp_controller/config/tests/test_controller.py
• openwisp_controller/config/tests/test_vpn.py

📜 Recent review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Python==3.11 | django~=5.1.0

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}

📄 CodeRabbit inference engine (Custom checks)

**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously

Files:

• openwisp_controller/config/base/vpn.py
• openwisp_controller/config/tests/test_vpn.py
• openwisp_controller/config/tests/test_controller.py

**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}

📄 CodeRabbit inference engine (Custom checks)

Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries

Files:

• openwisp_controller/config/base/vpn.py
• openwisp_controller/config/tests/test_vpn.py
• openwisp_controller/config/tests/test_controller.py

**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}

📄 CodeRabbit inference engine (Custom checks)

Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable

Files:

• openwisp_controller/config/base/vpn.py
• openwisp_controller/config/tests/test_vpn.py
• openwisp_controller/config/tests/test_controller.py

**/*.{py,html}

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework

Files:

• openwisp_controller/config/base/vpn.py
• openwisp_controller/config/tests/test_vpn.py
• openwisp_controller/config/tests/test_controller.py

🧠 Learnings (4)

📚 Learning: 2026-01-15T15:05:49.557Z

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.

Applied to files:

• openwisp_controller/config/base/vpn.py
• openwisp_controller/config/tests/test_vpn.py
• openwisp_controller/config/tests/test_controller.py

📚 Learning: 2026-02-17T19:13:10.088Z

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.

Applied to files:

• openwisp_controller/config/base/vpn.py
• openwisp_controller/config/tests/test_vpn.py
• openwisp_controller/config/tests/test_controller.py

📚 Learning: 2026-01-15T15:07:17.354Z

Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.

Applied to files:

• openwisp_controller/config/base/vpn.py
• openwisp_controller/config/tests/test_vpn.py
• openwisp_controller/config/tests/test_controller.py

📚 Learning: 2026-01-12T22:27:48.342Z

Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.

Applied to files:

• openwisp_controller/config/tests/test_controller.py

🔇 Additional comments (4)

openwisp_controller/config/base/vpn.py (1)

895-909: Non-mutating common-name construction is correct and safe.

This fixes the shared-instance mutation bug while keeping custom COMMON_NAME_FORMAT compatibility intact.

openwisp_controller/config/tests/test_vpn.py (2)

417-425: Updated long-name assertion and non-mutation check are solid.

This keeps the CN expectation aligned with truncation behavior and explicitly verifies Device.name remains unchanged.

---

426-470: New regression tests are well-targeted.

These tests correctly guard both in-memory and persisted name integrity for long and short hostname paths.

openwisp_controller/config/tests/test_controller.py (1)

1292-1335: Integration coverage for registration hostname integrity is excellent.

This test validates the exact end-to-end path that previously allowed silent name corruption.

---

📝 Walkthrough

Walkthrough

The PR fixes a bug in AbstractVpn._get_common_name that previously mutated Device.name in-place. The implementation now copies the device name into a local variable, builds an explicit formatting context, applies truncation to the formatted common name, and avoids changing the in-memory Device object. Tests were added in two test files to assert that device.name is not mutated during VPN certificate provisioning for long and short names (note: the registration test appears duplicated in the test file).

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• [bug] _get_common_name() mutates device name in memory during certificate creation #1284: Fixes _get_common_name mutating device.name and adds tests to prevent regression.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: preventing in-place mutation of device name in _get_common_name(). It follows the required [fix] format and uses descriptive, relevant terminology matching the bug description.
Description check	✅ Passed	The PR description comprehensively addresses the bug, fix, edge cases, tests, and impact. While it does not strictly follow the template sections (Checklist, Reference to Issue, etc.), it provides all essential information through well-organized narrative sections explaining the problem, solution, and verification.
Bug Fixes	✅ Passed	Root cause (in-place mutation of device.name) is fixed by using local variable and context dictionary approach that preserves device object integrity.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Tip

CodeRabbit can use your project's `ruff` configuration to improve the quality of Python code reviews.

Add a Ruff configuration file to your project to customize how CodeRabbit runs ruff.

[openwisp-companion]

CI Failures: Black, Commit Message, Test Failure

Hello @prakash-kalwaniya,
(Analysis for commit e010474)

1. Code Style/QA: The Black check failed. This indicates a formatting issue in the code.

• Fix: Run openwisp-qa-format to automatically fix the formatting.

2. Commit Message: The commit message validation failed.

• Fix: Ensure your commit message follows the OpenWISP convention:

[tag] Capitalized short title #<issue>

Body explaining the changes in detail.

Specifically, the commit title needs to start with a capital letter after the [fix] tag.

3. Test Failure: The test test_get_common_name_does_not_mutate_device_name in openwisp_controller/config/tests/test_vpn.py failed. The diff shows that the assertion self.assertIn(f"{mac}-{device_name}"[:-9], client._get_common_name()) was changed to self.assertIn(f"{mac}-{device_name}"[:-9], client._get_common_name()). This suggests the test might be too brittle or the change in the code is intended to fix a bug. However, the failure itself is likely due to the code style or commit message issues mentioned above, which might be preventing the test from passing correctly.

Please address the code style and commit message issues first, then re-run the CI to see if the test passes. If the test still fails, further investigation into the test logic might be needed.

[coveralls]

coverage: 98.658%. remained the same
when pulling be3f41a on prakash-kalwaniya:fix/vpn-get-common-name-device-mutation
into 8cf6733 on openwisp:master.

[nemesifier]

What issue is this solving? It seems like too much code for an issue that is not even listed in our issue list. Not a priority.

[nemesifier]

Unsolicited