Fix: avoid mutating device.name in _get_common_name

[k-sumayya]

Fix: Avoid in-place mutation of device.name in _get_common_name

This PR fixes an issue where _get_common_name was mutating device.name directly, causing unintended side effects due to Django ORM caching.

Changes:

• Removed in-place mutation of device.name
• Introduced a local variable name to safely handle truncation
• Ensured formatting uses the derived value instead of modifying the original object

Result:

• Prevents silent data corruption
• Keeps original device.name unchanged
• Aligns with expected behavior

Closes #1296

[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: 85bf4aec-e423-4753-aef4-06e1d6aa81c4

📥 Commits

Reviewing files that changed from the base of the PR and between 4510266 and 46acb5f.

📒 Files selected for processing (1)

• openwisp_controller/config/base/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.1.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=4.2.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

**/*.{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

**/*.{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

**/*.{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

🧠 Learnings (3)

📚 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

📚 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

📚 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

🔇 Additional comments (2)

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

895-898: LGTM! Mutation prevented correctly.

The introduction of the local name variable successfully prevents in-place mutation of device.name. The conditional at line 898 now correctly uses the truncated name variable instead of the mutated device attribute, preserving the original device.name in memory while achieving the same certificate Common Name logic.

---

900-900: Default COMMON_NAME_FORMAT works correctly with this implementation.

The format call cn_format.format(**{**d.__dict__, "name": name}) safely accesses database fields since both mac_address and name are stored fields in the Device model. The change itself fixes issue #1300 by preventing mutation of the original device.name, which is the intended purpose.

However, custom COMMON_NAME_FORMAT values referencing model properties (e.g., @property methods) or related field notation (e.g., {organization.name}) will raise KeyError since only database fields exist in __dict__. This is a pre-existing limitation not introduced by this commit. If custom formats need to support properties or relationships, consider refactoring to use a safer approach that computes the format dictionary explicitly rather than spreading __dict__.

---

📝 Walkthrough

Walkthrough

This change modifies AbstractVpnClient._get_common_name() to avoid mutating the Device instance when truncating long names. The truncated value is stored in a local name variable and used for the {mac_address}-{name} check and when formatting COMMON_NAME_FORMAT (cn_format.format(**{**d.__dict__, "name": name})). The final common_name remains capped and suffixed with the generated unique_slug, while d.name is left unchanged.

Sequence Diagram(s)

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related issues

• [bug] _get_common_name() mutates device name in memory during certificate creation #1284 — Change touches AbstractVpnClient._get_common_name and prevents in-place mutation of d.name by using a local truncated name, addressing the same mutation concern.

---

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Bug Fixes	❓ Inconclusive	Unable to access git repository or test files to verify the changes and test coverage for device name mutation in VPN configuration.	Provide git diff output and test file contents to verify if _get_common_name properly handles device name mutations and if tests cover this scenario.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title follows the required format with [fix] prefix and clearly describes the main change: avoiding mutation of device.name in _get_common_name.
Description check	✅ Passed	The PR description covers the issue, changes made, and results. However, it is missing the checklist items and screenshot section from the template.
Linked Issues check	✅ Passed	The PR successfully addresses all objectives from issue #1296: eliminates in-place mutation, uses a local variable for truncation, and preserves original device.name.
Out of Scope Changes check	✅ Passed	All changes in the PR are directly related to fixing the device.name mutation issue in _get_common_name(), with no out-of-scope modifications detected.

✨ 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.}

[openwisp-companion]

Commit Message Format Failure

Hello @k-sumayya,
(Analysis for commit c392985)

The CI failed because the commit message does not follow the required format. Please ensure your commit messages adhere to the following structure:

• Header: [prefix] Capitalized title or [prefix] Capitalized title #<issue>
• Body: A blank line after the header, followed by a detailed description.
• Footer: A closing keyword and issue number (e.g., Fixes #<issue>).

Here is an example of a correctly formatted commit message:

[fix] Update user authentication logic #123

This commit addresses an issue where users were unable to log in due to an incorrect password hashing algorithm. The hashing algorithm has been updated to bcrypt, and all existing user passwords have been rehashed.

Fixes #123

[coveralls]

coverage: 98.685% (+0.03%) from 98.658%
when pulling 46acb5f on k-sumayya:fix-common-name-bug
into 45b24b6 on openwisp:master.

[k-sumayya]

Thank you for the review! Please let me know if anything else is needed from my side.

[nemesifier]

Good catch @k-sumayya, this is a correct and worthwhile fix. The old code did d.name = d.name[:end], mutating the in-memory device instance as a side effect of generating a VPN common name. Because that object can be reused and potentially saved later in the same request, this could silently truncate and persist the device's actual name. Switching to a local name variable removes the side effect while keeping identical output.

One thing before merge:

• Please add a regression test asserting that after _get_common_name() runs, device.name is unchanged (and that the generated common name is still correct, including the {mac_address}-{name} collapse-to-{mac_address} branch). This is exactly the kind of side-effect bug that silently comes back without a test pinning it.

Correct fix, just add the test and it is ready.