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]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

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: 36e5963a-86a7-4121-9f01-cc3e10ffe2f7

📥 Commits

Reviewing files that changed from the base of the PR and between 520e8a0 and 715c852.

📒 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.11 | django~=5.2.0
• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=4.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~=4.2.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=5.1.0

🧰 Additional context used

📓 Path-based instructions (5)

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

**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Mark user-facing strings for translation with Django i18n helpers in Django code
Avoid unnecessary blank lines inside function and method bodies
Be careful with authentication, authorization, queryset filtering, serializers, admin behavior, cache invalidation, signals, Celery tasks, and websocket updates in Django code
Preserve validation around templates, VPN/PKI material, SSH credentials, device commands, uploaded files, URLs, and subnet/IP data
Write comments and docstrings only when they explain why code is shaped a certain way, placing them before the relevant code block instead of scattering them inside it

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)

889-902: LGTM!

---

253-254: Major: Ensure old VPN client certs are revoked/deleted when CA changes

The CA-change logic replaces self.cert when self.cert.ca_id != self.ca_id, but the current code snippet doesn’t show any revocation/deletion of the replaced cert; if the old cert remains valid for the previous CA, that’s a security risk (compare with existing cert revocation behavior used on VpnClient deletion/deactivation).

---

📝 Walkthrough

Walkthrough

AbstractVpnClient._get_common_name() was changed to truncate the device name into a local variable instead of mutating d.name. The local truncated name is used for the {mac_address}-{name} check and injected into COMMON_NAME_FORMAT via cn_format.format(**{**d.dict, "name": name}); the final common_name is then capped and suffixed with unique_slug. The test now asserts d.name is unchanged after calling the method.

Sequence Diagram(s)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• [bug] _get_common_name() mutates device name in memory during certificate creation #1284 — Fixes the same in-place mutation of device.name in _get_common_name by using a local truncated name.
• [fix] Device name corrupted by in-place mutation in _get_common_name() during VPN cert provisioning #1296 — Matches the bug report describing Device.name corruption from in-place truncation; this change implements the suggested non-mutating approach.

Suggested labels

enhancement

Suggested reviewers

• nemesifier

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly identifies the fix and matches the issue's core problem - preventing mutation of device.name in _get_common_name.
Description check	✅ Passed	Description covers the problem, changes made, and results, though it lacks a screenshot section which is non-critical.
Linked Issues check	✅ Passed	Changes fully satisfy issue #1296 requirements: removed in-place mutation, introduced local variable for truncation, preserved original device.name, and prevent side effects.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the device.name mutation issue in _get_common_name() and corresponding test updates, with no out-of-scope modifications.
Bug Fixes	✅ Passed	_get_common_name truncates into local name and formats CN without writing back to d.name; test_auto_create_cert_with_long_device_name asserts d.name unchanged and CN base (excluding random suffix...

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

[openwisp-companion]

Hi @k-sumayya 👋,

This is a friendly reminder that this pull request has had no activity for 8 days since changes were requested.

We'd love to see this contribution merged! Please take a moment to:

• Address the review feedback
• Push your changes
• Let us know if you have any questions or need clarification

If you're busy or need more time, no worries! Just leave a comment to let us know you're still working on it.

Note: within 6 more days, the linked issue will be unassigned to allow other contributors to work on it.

Thank you for your contribution! 🙏

[openwisp-companion]

Hi @k-sumayya 👋,

This pull request has been marked as stale due to 14 days of inactivity after changes were requested.

As a result, any linked issues are being unassigned from you so other contributors can pick them up.

However, you can still continue working on this PR! If you push new commits or respond to the review feedback:

• The issue will be reassigned to you
• Your contribution is still very welcome

If you need more time or have questions about the requested changes, please let us know. We're happy to help! 🤝

[openwisp-companion]

Dependency Conflict During Package Installation

Hello @k-sumayya,
(Analysis for commit 520e8a0)

There is a dependency conflict between openwisp-controller and openwisp-ipam packages during the installation process.

Failure:

The CI failed because openwisp-controller==1.3a0 depends on django-reversion~=6.0.0, while openwisp-ipam==1.3a0 depends on django-reversion~=6.2.0. Pip cannot resolve these conflicting version requirements.

To Fix:

Update the version specifier for django-reversion in the requirements-test.txt file to a compatible version that satisfies both dependencies. For example, you could try django-reversion~=6.2.0 or a more relaxed range if appropriate.

[openwisp-companion]

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3).

[openwisp-companion]

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (2/3).

[openwisp-companion]

The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (3/3).

[openwisp-companion]

Test Failures in Selenium Tests

Hello @k-sumayya,
(Analysis for commit 715c852)

The CI build failed due to a test error in TestDevice.test_restoring_deleted_device.

Failure:
A TypeError: 'NoneType' object is not iterable occurred in the test test_restoring_deleted_device within the TestDevice class. This indicates that a function expected to return an iterable (like a list or tuple) returned None instead.

Location:
The error originates from the line for error in self.get_browser_logs(): in the test file openwisp_controller/tests/test_selenium.py. This suggests that self.get_browser_logs() returned None.

Fix:
The get_browser_logs method in the TestDevice class likely needs to be reviewed to ensure it always returns an iterable, even if there are no logs. It's possible that in certain test conditions, it's returning None when it should return an empty list.

Additionally, several other Selenium tests are reported as slow, which is a separate issue that might need attention after the primary failure is resolved.

[k-sumayya]

Added the requested regression test to ensure device.name remains unchanged after _get_common_name() runs. The remaining CI failure appears to be in the unrelated Selenium test TestDevice.test_restoring_deleted_device

[openwisp-companion]

Thanks for following up, @k-sumayya! 🙌 The stale status has been removed and the linked issue(s) have been reassigned to you. Looking forward to your updates!