[fix] Decouple OpenVPN config filename from VPN name #572#609
[fix] Decouple OpenVPN config filename from VPN name #572#609nemesifier wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR fixes a bug where OpenVPN configurations with whitespace in the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ 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 |
Code Review SummaryStatus: No Critical Issues Found | Recommendation: LGTM, ready to merge Security Improvements Observed
Files Reviewed
SummaryThe changes are minimal, focused, and security-conscious. The refactor from unquoted variables to properly quoted ones eliminates potential command injection. The cleanup-on-error pattern is consistent throughout. Action: Approved for merge. Reviewed by kimi-k2.5-0127 · 170,054 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@images/common/utils.sh`:
- Around line 261-263: The current logic in images/common/utils.sh that sets
CONF_FILE by using find ... -print -quit (when CONF_FILE doesn't exist) can pick
an arbitrary .conf when multiple are present; change it to explicitly collect
all matches from TMPDIR (e.g., into an array or list), then: if exactly one
match set CONF_FILE to that path, if zero leave CONF_FILE unset, and if more
than one log an error and exit (fail fast). Apply the same change to the other
similar block referenced (lines 264-268) so both places check the number of
.conf files and error out deterministically instead of using find -print -quit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9def431-699b-4f71-8350-5d24fdc1a223
📒 Files selected for processing (4)
images/common/utils.shimages/openwisp_openvpn/openvpn.shimages/openwisp_openvpn/supervisord.conftests/runtests.py
📜 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). (3)
- GitHub Check: auto-assign-issue / run-bot
- GitHub Check: CI Build
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
tests/runtests.py
🧠 Learnings (1)
📚 Learning: 2026-02-17T12:50:25.569Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 564
File: images/common/utils.sh:33-40
Timestamp: 2026-02-17T12:50:25.569Z
Learning: For shell scripts under images/common that invoke certbot, in certbot >= 3.3.0 (Mar 2025), you no longer need --register-unsafely-without-email when using --noninteractive --agree-tos without providing an email. If your scripts previously passed this flag, remove it to rely on default account registration without an email. This applies when no email is supplied; if an email is provided, behavior is unchanged. Update tests to reflect that certbot will proceed without prompting for an email in non-interactive mode.
Applied to files:
images/common/utils.sh
🔇 Additional comments (3)
images/openwisp_openvpn/supervisord.conf (1)
21-21: LGTM!images/openwisp_openvpn/openvpn.sh (1)
12-12: LGTM!tests/runtests.py (1)
13-13: LGTM!Also applies to: 61-113
| if [ ! -f "$CONF_FILE" ]; then | ||
| CONF_FILE=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' -print -quit) | ||
| fi |
There was a problem hiding this comment.
Fail fast when multiple extracted .conf files are present.
find ... -print -quit picks an arbitrary first match when there are multiple .conf files (and no pre-existing openvpn.conf), which can select the wrong profile non-deterministically.
Suggested fix
CONF_FILE="$TMPDIR/openvpn.conf"
if [ ! -f "$CONF_FILE" ]; then
- CONF_FILE=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' -print -quit)
+ CONF_COUNT=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' | wc -l)
+ if [ "$CONF_COUNT" -ne 1 ]; then
+ echo "ERROR: expected exactly one OpenVPN config file, found $CONF_COUNT" >&2
+ rm -rf -- "$TMPDIR"
+ return 1
+ fi
+ CONF_FILE=$(find "$TMPDIR" -maxdepth 1 -type f -name '*.conf' -print -quit)
fi
if [ -z "$CONF_FILE" ]; thenAlso applies to: 264-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@images/common/utils.sh` around lines 261 - 263, The current logic in
images/common/utils.sh that sets CONF_FILE by using find ... -print -quit (when
CONF_FILE doesn't exist) can pick an arbitrary .conf when multiple are present;
change it to explicitly collect all matches from TMPDIR (e.g., into an array or
list), then: if exactly one match set CONF_FILE to that path, if zero leave
CONF_FILE unset, and if more than one log an error and exit (fail fast). Apply
the same change to the other similar block referenced (lines 264-268) so both
places check the number of .conf files and error out deterministically instead
of using find -print -quit.
1 participant
Checklist
Reference to Existing Issue
Closes #572.
Description of Changes
This fixes the OpenVPN startup failure that happens when
VPN_NAMEcontains whitespace.The OpenVPN process managed by supervisord now always starts from a fixed
openvpn.conffilename instead of deriving the config path fromVPN_NAME. The downloaded VPN archive is extracted in an isolated temporary directory, then its.conffile is normalized toopenvpn.confso stale files in/cannot be reused accidentally.The cron update script now restarts OpenVPN only after a successful config download. A container-backed regression test was added for a downloaded config named
my vpn.conf.