[change] Made Site name configuration and uppdated OpenVPN template#566
[change] Made Site name configuration and uppdated OpenVPN template#566
Conversation
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
pandafy
moved this from To do (general)
to Needs review
in OpenWISP Contributor's Board
📝 WalkthroughWalkthroughAdded a conditional default OpenVPN client log path in the load-init script: when a Template with an openvpn config is created, its config is validated and, if present, config["openvpn"][0]["log"] is set to "/var/log/tun0.log" before save. Introduced a new public function update_default_site() that reads DASHBOARD_DOMAIN from the environment and updates the Django Sites default entry when the current site name or domain equals "example.com". update_default_site() is invoked after admin creation in create_default_topology(vpn) and in the script main flow after admin creation. The OpenVPN JSON replaces "cipher" with explicit "cipher": "AES-128-GCM" and adds "data_ciphers": "AES-128-GCM:none". Sequence Diagram(s)sequenceDiagram
participant Script
participant Template
participant AdminCreator
participant Env
participant Sites
Script->>Template: instantiate Template(...) and full_clean()
Template->>Template: if config.openvpn exists
Template->>Template: set config.openvpn[0].log = "/var/log/tun0.log"
Template->>Template: save()
Script->>AdminCreator: create admin user
AdminCreator-->>Script: admin created
Script->>Env: read DASHBOARD_DOMAIN
Script->>Sites: query Site where domain or name == "example.com"
alt match found
Script->>Sites: update Site.domain and Site.name = DASHBOARD_DOMAIN
Sites-->>Script: save confirmation
else
Sites-->>Script: no change
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
images/openwisp_dashboard/load_init_data.py (2)
205-205: Usesettings.SITE_IDto reliably identify the default site instead of.first().
Site.objects.first()returns the lowest-PK site, which may not be the site identified bysettings.SITE_IDin multi-site deployments. UseSite.objects.get_current()(which respectssettings.SITE_IDand the request-cache) or the explicit lookup below:♻️ Proposed fix
- site = Site.objects.first() + site = Site.objects.get(id=settings.SITE_ID)You would also need to handle the
Site.DoesNotExistexception ifSITE_IDpoints to a missing record:+ try: + site = Site.objects.get(id=settings.SITE_ID) + except Site.DoesNotExist: + return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/load_init_data.py` at line 205, Replace the fragile Site.objects.first() lookup with a SITE_ID-aware lookup: import django.conf.settings and use Site.objects.get_current() (which respects settings.SITE_ID) or Site.objects.get(pk=settings.SITE_ID) inside the same function where site is needed (e.g., around the Site variable assignment in load_init_data.py). Add a try/except for Site.DoesNotExist to handle a missing SITE_ID record and log or raise a clear error, ensuring subsequent code uses the resolved site object.
208-210: Callsite.full_clean()beforesite.save()to validate the env-sourced domain.
dashboard_domaincomes fromos.environwithout any format validation. Callingsite.full_clean()would apply Django's built-inURLValidator/field-level validators on the domain before it is persisted, preventing malformed values from being silently written to the database.♻️ Proposed fix
site.name = dashboard_domain site.domain = dashboard_domain + site.full_clean() site.save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/load_init_data.py` around lines 208 - 210, The code assigns dashboard_domain (from os.environ) directly to site.name and site.domain then calls site.save(); instead call site.full_clean() before saving to run Django field validators (including URL/Domain validators) and raise/handle ValidationError for malformed env values—so after setting site.name and site.domain invoke site.full_clean() and only then site.save(), ensuring you catch or log ValidationError for dashboard_domain as needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
images/openwisp_dashboard/load_init_data.pyimages/openwisp_dashboard/openvpn.json
🧰 Additional context used
🪛 GitHub Check: CodeQL
images/openwisp_dashboard/load_init_data.py
[failure] 207-207: Incomplete URL substring sanitization
The string example.com may be at an arbitrary position in the sanitized URL.
⏰ 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). (1)
- GitHub Check: CI Build
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Line 207: The conditional using `"example.com" in [site.name, site.domain]`
should be replaced with explicit equality checks to avoid the CodeQL false
positive; update the if in load_init_data.py so it directly compares site.name
== "example.com" or site.domain == "example.com" (while preserving the existing
`site` and `dashboard_domain` checks) so intent is unambiguous and behavior
unchanged.
In `@images/openwisp_dashboard/openvpn.json`:
- Around line 14-15: Change the incorrect OpenVPN config field and format:
replace the unrecognized "data_cipher" key with "data_ciphers" and convert its
string value into an array of cipher objects (each object containing "cipher"
and "optional" keys) instead of the colon-separated string; also remove the
"none" (unencrypted) option so only valid encrypted ciphers like "AES-128-GCM"
are listed. Ensure the final "data_ciphers" is an array of objects (e.g., {
"cipher": "...", "optional": false }) to match the netjsonconfig v1.1.0+ OpenVPN
schema.
---
Nitpick comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Line 205: Replace the fragile Site.objects.first() lookup with a SITE_ID-aware
lookup: import django.conf.settings and use Site.objects.get_current() (which
respects settings.SITE_ID) or Site.objects.get(pk=settings.SITE_ID) inside the
same function where site is needed (e.g., around the Site variable assignment in
load_init_data.py). Add a try/except for Site.DoesNotExist to handle a missing
SITE_ID record and log or raise a clear error, ensuring subsequent code uses the
resolved site object.
- Around line 208-210: The code assigns dashboard_domain (from os.environ)
directly to site.name and site.domain then calls site.save(); instead call
site.full_clean() before saving to run Django field validators (including
URL/Domain validators) and raise/handle ValidationError for malformed env
values—so after setting site.name and site.domain invoke site.full_clean() and
only then site.save(), ensuring you catch or log ValidationError for
dashboard_domain as needed.
| "data_cipher": "AES-128-GCM:none", | ||
| "cipher": "AES-128-GCM", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "openvpn.json" -type fRepository: openwisp/docker-openwisp
Length of output: 108
🏁 Script executed:
# Search for netjsonconfig references and schema definitions
fd -e json -e py -e md "netjson" --exec grep -l "data.cipher" {} \;
rg -n "data.cipher|data-cipher" --type py --type json -C 2 | head -100Repository: openwisp/docker-openwisp
Length of output: 415
🏁 Script executed:
# Look for netjsonconfig configuration in the repo
fd -type f -name "*.py" | xargs grep -l "netjsonconfig\|openvpn" | head -10Repository: openwisp/docker-openwisp
Length of output: 238
🏁 Script executed:
# Search for netjsonconfig in the repository
rg "netjsonconfig|data.cipher|data-cipher" -C 3 | head -150Repository: openwisp/docker-openwisp
Length of output: 2262
🏁 Script executed:
# Look for python files that might contain configuration or schema
find . -name "*.py" -type f | xargs grep -l "data_cipher\|data-cipher" 2>/dev/nullRepository: openwisp/docker-openwisp
Length of output: 52
🏁 Script executed:
# Check if there are any netjsonconfig configuration files or documentation
find . -name "*.md" -o -name "*.txt" -o -name "requirements*.txt" | xargs grep -l "netjsonconfig" 2>/dev/null | head -5Repository: openwisp/docker-openwisp
Length of output: 50
🏁 Script executed:
# Look at the openvpn.json file in context to understand the structure
cat -n ./images/openwisp_dashboard/openvpn.json | head -30Repository: openwisp/docker-openwisp
Length of output: 970
🏁 Script executed:
# Search for any documentation or references to netjsonconfig schema
rg "openvpn" -t md -C 3 | head -100Repository: openwisp/docker-openwisp
Length of output: 216
🏁 Script executed:
# Check if there's a setup.py or requirements file that shows netjsonconfig version
cat requirements*.txt setup.py 2>/dev/null | grep -i netjsonconfigRepository: openwisp/docker-openwisp
Length of output: 50
🏁 Script executed:
# Look for any comments or documentation about the openvpn.json file
find . -name "*.md" -o -name "*.rst" | xargs grep -l "openvpn\|data_cipher" 2>/dev/nullRepository: openwisp/docker-openwisp
Length of output: 204
🏁 Script executed:
# Check the load_init_data.py to understand how openvpn.json is used
cat -n ./images/openwisp_dashboard/load_init_data.py | grep -A 20 -B 5 "openvpn"Repository: openwisp/docker-openwisp
Length of output: 3841
🌐 Web query:
netjsonconfig OpenVPN data_cipher data-ciphers field name schema
💡 Result:
In netjsonconfig’s OpenVPN backend schema, the OpenVPN --data-ciphers option is represented by the snake_case field:
data_ciphers: a list of objects, where each object must contain:cipher(string)optional(boolean) (netjsonconfig.openwisp.org)
Related field:
data_ciphers_fallback: a string (maps to OpenVPN--data-ciphers-fallback). (netjsonconfig.openwisp.org)
So the schema field name is data_ciphers (not data_cipher, and not data-ciphers). netjsonconfig added support for this option in v1.1.0. (netjsonconfig.openwisp.org)
Citations:
- 1: https://netjsonconfig.openwisp.org/en/stable/backends/openvpn.html?utm_source=openai
- 2: https://netjsonconfig.openwisp.org/en/stable/backends/openvpn.html?utm_source=openai
- 3: https://netjsonconfig.openwisp.org/en/master/general/changelog.html?utm_source=openai
🌐 Web query:
netjsonconfig openvpn json schema documentation
💡 Result:
- Primary documentation (OpenVPN backend + schema overview): netjsonconfig “OpenVPN Backend” docs (includes the “OpenVPN backend schema” section and links to the schema source). [1]
- OpenVPN JSON-Schema source (authoritative schema definition used by the backend):
netjsonconfig/backends/openvpn/schema.pyin theopenwisp/netjsonconfigrepo (raw source). [2] - How schemas/validation work in netjsonconfig (general docs): “Basic concepts” (schema inheritance +
validatebehavior). [3] - Project entry point / repo (for browsing other backend schemas and docs versions):
openwisp/netjsonconfigon GitHub. [4] - Underlying NetJSON spec note (why unknown properties may be ignored): NetJSON RFC/draft text describing how unrecognized custom members should be handled. [5]
Sources: [1] (netjsonconfig.openwisp.org) [2] (raw.githubusercontent.com) [3] (netjsonconfig.openwisp.org) [4] (github.com) [5] (netjson.org)
Citations:
- 1: https://netjsonconfig.openwisp.org/en/stable/backends/openvpn.html?utm_source=openai
- 2: https://raw.githubusercontent.com/openwisp/netjsonconfig/master/netjsonconfig/backends/openvpn/schema.py
- 3: https://netjsonconfig.openwisp.org/en/latest/general/basics.html?utm_source=openai
- 4: https://github.com/openwisp/netjsonconfig?utm_source=openai
- 5: https://netjson.org/rfc.html?utm_source=openai
🏁 Script executed:
# Check if there's a CHANGES.rst or documentation about field names
cat ./CHANGES.rst | grep -A 5 -B 5 "data.cipher\|netjsonconfig"Repository: openwisp/docker-openwisp
Length of output: 698
🏁 Script executed:
# Look for any validation or schema comments in Python files
rg "data_cipher|data-cipher" -B 3 -A 3Repository: openwisp/docker-openwisp
Length of output: 519
🌐 Web query:
netjsonconfig data_ciphers format list objects cipher optional example
💡 Result:
In netjsonconfig’s OpenVPN backend, the data_ciphers field is:
- a list (array) of objects (dicts)
- each object must contain:
cipher(string)optional(boolean)
as described in the OpenVPN backend schema docs. [1]
(data_ciphers_fallbackis a single string cipher name.) [1]
Example (NetJSONConfig / OpenVPN instance)
{
"openvpn": [
{
"name": "example-vpn",
"mode": "server",
"proto": "udp",
"dev": "tun0",
"dev_type": "tun",
"data_ciphers": [
{ "cipher": "AES-256-GCM", "optional": false },
{ "cipher": "AES-128-GCM", "optional": true }
],
"data_ciphers_fallback": "AES-256-CBC"
}
]
}(Choose cipher names from the OpenVPN backend’s allowed cipher list referenced by the docs.) [1]
Note: data-ciphers support was added to netjsonconfig in v1.1.0 (2024-11-22). [2]
Sources:
[1] netjsonconfig OpenVPN backend docs (schema table including data_ciphers) https://netjsonconfig.openwisp.org/en/stable/backends/openvpn.html
[2] netjsonconfig changelog (feature: “Added data-ciphers option for OpenVPN”) https://netjsonconfig.openwisp.org/en/stable/general/changelog.html
🏁 Script executed:
# Check if there are any test files or examples in the repo showing correct format
find . -name "*.json" -o -name "*.py" | xargs grep -l "data_cipher" 2>/dev/nullRepository: openwisp/docker-openwisp
Length of output: 108
Fix field name, format, and remove unencrypted cipher option.
The configuration has three critical issues:
-
Wrong field name:
data_ciphershould bedata_ciphers(plural). With the incorrect name, netjsonconfig silently ignores the entire field, meaning the data cipher configuration is never applied. -
Wrong format: The field expects an array of objects (each with
cipherandoptionalkeys), not a colon-separated string. Current format"AES-128-GCM:none"is invalid for netjsonconfig's OpenVPN schema. -
Security issue: The value
"none"disables data channel encryption and is only for debugging. Shipping it in production allows clients to transmit VPN traffic in cleartext.
Correct the configuration:
Fix
- "data_cipher": "AES-128-GCM:none",
+ "data_ciphers": [
+ {"cipher": "AES-128-GCM", "optional": false}
+ ],(Per netjsonconfig v1.1.0+ OpenVPN backend schema, data_ciphers is a list of cipher objects; data_cipher is not a recognized field and will be silently ignored.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "data_cipher": "AES-128-GCM:none", | |
| "cipher": "AES-128-GCM", | |
| "data_ciphers": [ | |
| {"cipher": "AES-128-GCM", "optional": false} | |
| ], | |
| "cipher": "AES-128-GCM", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/openwisp_dashboard/openvpn.json` around lines 14 - 15, Change the
incorrect OpenVPN config field and format: replace the unrecognized
"data_cipher" key with "data_ciphers" and convert its string value into an array
of cipher objects (each object containing "cipher" and "optional" keys) instead
of the colon-separated string; also remove the "none" (unencrypted) option so
only valid encrypted ciphers like "AES-128-GCM" are listed. Ensure the final
"data_ciphers" is an array of objects (e.g., { "cipher": "...", "optional":
false }) to match the netjsonconfig v1.1.0+ OpenVPN schema.
400f4d0 to
a586871
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
images/openwisp_dashboard/load_init_data.py (1)
205-205: PreferSite.objects.get(pk=settings.SITE_ID)overSite.objects.first().
first()may return the wrong site when multiple sites exist. TheSITE_IDsetting unambiguously identifies the "current" site intended by Django's sites framework.♻️ Proposed refactor
- site = Site.objects.first() + site = Site.objects.get(pk=settings.SITE_ID)Then simplify the guard (the object is guaranteed to exist):
- if site and "example.com" in [site.name, site.domain] and dashboard_domain: + if (site.name == "example.com" or site.domain == "example.com") and dashboard_domain:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/load_init_data.py` at line 205, Replace the fragile query Site.objects.first() with an explicit lookup using Site.objects.get(pk=settings.SITE_ID): import settings if not present, call Site.objects.get(pk=settings.SITE_ID) where the variable site is assigned, and then remove or simplify any subsequent existence guard (since SITE_ID should always resolve to an existing Site); reference the Site model and settings.SITE_ID in the change and ensure any DoesNotExist handling is adjusted or removed accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
images/openwisp_dashboard/load_init_data.pyimages/openwisp_dashboard/openvpn.json
🚧 Files skipped from review as they are similar to previous changes (1)
- images/openwisp_dashboard/openvpn.json
🧰 Additional context used
🪛 GitHub Check: CodeQL
images/openwisp_dashboard/load_init_data.py
[failure] 207-207: Incomplete URL substring sanitization
The string example.com may be at an arbitrary position in the sanitized URL.
⏰ 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). (1)
- GitHub Check: CI Build
🔇 Additional comments (1)
images/openwisp_dashboard/load_init_data.py (1)
255-256: LGTM — placement ofupdate_default_site()is correct.Calling it right after
create_admin()and before VPN setup ensures the site domain is updated before any subsequent steps that might depend on it. The inline conditional import inside the function keeps it self-contained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Around line 136-138: The code unguardedly accesses
template.config['openvpn'][0], which can raise KeyError/IndexError; update the
block around template.full_clean(), template.config and template.save() to
defensively ensure template.config has an 'openvpn' key that is a non-empty list
(e.g., if 'openvpn' missing or not a list, set it to a list with at least one
dict), then set the first element's 'log' value to '/var/log/tun0.log' and call
template.save(); reference the existing symbols template.config,
template.full_clean, and template.save when making this change.
---
Duplicate comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Line 207: Replace the substring membership check "example.com" in [site.name,
site.domain] with explicit equality comparisons to avoid the CodeQL false
positive: change the conditional that checks site and dashboard_domain (the one
using site.name and site.domain) to test site.name == "example.com" or
site.domain == "example.com" so the intent is unambiguous and substring matching
is eliminated.
---
Nitpick comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Line 205: Replace the fragile query Site.objects.first() with an explicit
lookup using Site.objects.get(pk=settings.SITE_ID): import settings if not
present, call Site.objects.get(pk=settings.SITE_ID) where the variable site is
assigned, and then remove or simplify any subsequent existence guard (since
SITE_ID should always resolve to an existing Site); reference the Site model and
settings.SITE_ID in the change and ensure any DoesNotExist handling is adjusted
or removed accordingly.
- Updated ciphers - Added login to OpenVPN template
pandafy
force-pushed
the
default-site-and-openvpn
branch
from
a586871 to
1b3ff7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
images/openwisp_dashboard/openvpn.json (1)
14-14:⚠️ Potential issue | 🔴 Critical
data_ciphersstill has the wrong value type, andnoneremains a security risk.The field name was corrected to
data_ciphers(plural), but the value"AES-128-GCM:none"is still a colon-separated string. Per the netjsonconfig OpenVPN schema,data_ciphersexpects a list of dicts, each with acipherandoptionalkey. A plain string is not a valid value and will be treated as an unrecognized type — according to the NetJSON spec, any unrecognized property will be ignored — so this field has no effect at runtime.Additionally,
nonein the cipher list disables data-channel encryption and should never ship in production config.🔧 Proposed fix
- "data_ciphers": "AES-128-GCM:none", + "data_ciphers": [ + {"cipher": "AES-128-GCM", "optional": false} + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@images/openwisp_dashboard/openvpn.json` at line 14, Replace the string value for the data_ciphers field with a list of dicts conforming to the NetJSON OpenVPN schema (each dict must have cipher and optional keys); e.g. remove the insecure "none" entry and use an array like [{"cipher":"AES-128-GCM","optional":false}] so data_ciphers is an actual list of objects rather than a colon-separated string and thus will be honored at runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@images/openwisp_dashboard/load_init_data.py`:
- Around line 206-216: Site.objects.get(pk=settings.SITE_ID) can raise
Site.DoesNotExist and never returns None, so remove the dead "if site" guard and
wrap the lookup in a try/except Site.DoesNotExist block; on exception, skip the
update (or log a clear message) instead of letting the init script crash, and
keep the existing update logic (assigning dashboard_domain to site.name and
site.domain, calling site.full_clean() and site.save()) inside the try block so
it only runs when the Site exists and dashboard_domain is set.
- Line 137: The conditional at the top of load_init_data.py redundantly checks
length; replace the expression if template.config.get("openvpn", []) and
len(template.config["openvpn"]) > 0 with a single truthy check using
template.config.get("openvpn") so the code reads simply to test for a non-empty
openvpn list; update any nearby comments to reflect the simplified check.
- Around line 136-139: The code calls template.full_clean(), then mutates
template.config by setting template.config["openvpn"][0]["log"] and immediately
calls template.save() which bypasses validation of the mutated state; update the
flow so the mutation is validated by either moving the openvpn log assignment
before the initial template.full_clean() or, if keeping the current order, call
template.full_clean() again after setting template.config["openvpn"][0]["log"]
and before template.save(); reference the existing symbols
template.full_clean(), template.config["openvpn"][0][\"log\"], and
template.save() when making the change.
---
Duplicate comments:
In `@images/openwisp_dashboard/openvpn.json`:
- Line 14: Replace the string value for the data_ciphers field with a list of
dicts conforming to the NetJSON OpenVPN schema (each dict must have cipher and
optional keys); e.g. remove the insecure "none" entry and use an array like
[{"cipher":"AES-128-GCM","optional":false}] so data_ciphers is an actual list of
objects rather than a colon-separated string and thus will be honored at
runtime.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
images/openwisp_dashboard/load_init_data.pyimages/openwisp_dashboard/openvpn.json
📜 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). (1)
- GitHub Check: CI Build
🔇 Additional comments (2)
images/openwisp_dashboard/load_init_data.py (2)
261-262: LGTM —update_default_site()is correctly placed aftercreate_admin().
219-242: AI summary inconsistency:update_default_site()is not called insidecreate_default_topology.The AI summary states that
update_default_site()is "invoked aftercreate_admin()in two locations: withincreate_default_topology(vpn)flow." The function at lines 219–242 contains no such call. The only invocation is at line 262 in the__main__block.
| template.full_clean() | ||
| if template.config.get("openvpn", []) and len(template.config["openvpn"]) > 0: | ||
| template.config["openvpn"][0]["log"] = "/var/log/tun0.log" | ||
| template.save() |
There was a problem hiding this comment.
Config mutated after full_clean() but saved without re-validation.
template.full_clean() is called at line 136, then config is modified at line 138, and template.save() is called at line 139 without a second full_clean(). The log path is simple and unlikely to break, but mutating validated state and then skipping re-validation is incorrect ordering; if the schema tightens, this silently bypasses it.
🔧 Proposed fix
template.full_clean()
if template.config.get("openvpn", []) and len(template.config["openvpn"]) > 0:
template.config["openvpn"][0]["log"] = "/var/log/tun0.log"
+ template.full_clean()
template.save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/openwisp_dashboard/load_init_data.py` around lines 136 - 139, The code
calls template.full_clean(), then mutates template.config by setting
template.config["openvpn"][0]["log"] and immediately calls template.save() which
bypasses validation of the mutated state; update the flow so the mutation is
validated by either moving the openvpn log assignment before the initial
template.full_clean() or, if keeping the current order, call
template.full_clean() again after setting template.config["openvpn"][0]["log"]
and before template.save(); reference the existing symbols
template.full_clean(), template.config["openvpn"][0][\"log\"], and
template.save() when making the change.
| default=True, | ||
| ) | ||
| template.full_clean() | ||
| if template.config.get("openvpn", []) and len(template.config["openvpn"]) > 0: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant length guard.
template.config.get("openvpn", []) already evaluates falsy for an empty list, making and len(template.config["openvpn"]) > 0 a no-op.
♻️ Proposed simplification
- if template.config.get("openvpn", []) and len(template.config["openvpn"]) > 0:
+ if template.config.get("openvpn"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/openwisp_dashboard/load_init_data.py` at line 137, The conditional at
the top of load_init_data.py redundantly checks length; replace the expression
if template.config.get("openvpn", []) and len(template.config["openvpn"]) > 0
with a single truthy check using template.config.get("openvpn") so the code
reads simply to test for a non-empty openvpn list; update any nearby comments to
reflect the simplified check.
| site = Site.objects.get(pk=settings.SITE_ID) | ||
| dashboard_domain = os.environ.get("DASHBOARD_DOMAIN", "") | ||
| if ( | ||
| site | ||
| and (site.name == "example.com" or site.domain == "example.com") | ||
| and dashboard_domain | ||
| ): | ||
| site.name = dashboard_domain | ||
| site.domain = dashboard_domain | ||
| site.full_clean() | ||
| site.save() |
There was a problem hiding this comment.
Unhandled Site.DoesNotExist and dead if site guard.
Site.objects.get(pk=settings.SITE_ID) either returns a Site instance or raises Site.DoesNotExist — it never returns None. Therefore:
- The
if sitecheck on line 209 is dead code and gives a false sense of safety. - If
settings.SITE_IDis not present in the database (e.g., first deploy before migrations), the unhandledDoesNotExistexception will crash the entire init script.
🔧 Proposed fix
- site = Site.objects.get(pk=settings.SITE_ID)
- dashboard_domain = os.environ.get("DASHBOARD_DOMAIN", "")
- if (
- site
- and (site.name == "example.com" or site.domain == "example.com")
- and dashboard_domain
- ):
- site.name = dashboard_domain
- site.domain = dashboard_domain
- site.full_clean()
- site.save()
+ dashboard_domain = os.environ.get("DASHBOARD_DOMAIN", "")
+ if not dashboard_domain:
+ return
+ try:
+ site = Site.objects.get(pk=settings.SITE_ID)
+ except Site.DoesNotExist:
+ return
+ if site.name == "example.com" or site.domain == "example.com":
+ site.name = dashboard_domain
+ site.domain = dashboard_domain
+ site.full_clean()
+ site.save()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@images/openwisp_dashboard/load_init_data.py` around lines 206 - 216,
Site.objects.get(pk=settings.SITE_ID) can raise Site.DoesNotExist and never
returns None, so remove the dead "if site" guard and wrap the lookup in a
try/except Site.DoesNotExist block; on exception, skip the update (or log a
clear message) instead of letting the init script crash, and keep the existing
update logic (assigning dashboard_domain to site.name and site.domain, calling
site.full_clean() and site.save()) inside the try block so it only runs when the
Site exists and dashboard_domain is set.
| "comp_lzo": "no", | ||
| "auth": "SHA1", | ||
| "cipher": "none", | ||
| "data_ciphers": "AES-128-GCM:none", |
There was a problem hiding this comment.
I think we need to add also data_ciphers_fallback which is the new cipher fallback on newer versions.
2 participants
Made Site name configuration and uppdated OpenVPN template