[qa] Enforce QA on shell scripts, JSON, md and YAML files#634
Conversation
nemesifier
added
enhancement
📝 WalkthroughWalkthroughInstalls shfmt and prettier in CI, updates the QA script to always run openwisp-qa-check with JS/CSS linters and to always run shfmt, refactors many image shell functions from non‑POSIX Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
images/common/utils.sh (1)
247-261:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQuote variables in URL construction.
Line 249: Variables
$DASHBOARD_INTERNALand$CA_UUIDshould be quoted to prevent word splitting and globbing.🔧 Proposed fix
crl_download() { curl --silent --insecure --output revoked.crl \ - ${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl + "${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl" }🤖 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 247 - 261, The curl URL in crl_download uses unquoted variables which can cause word splitting or globbing; update the command in crl_download to quote the composed URL by surrounding the ${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl expression with double quotes (e.g. "…${DASHBOARD_INTERNAL}/…${CA_UUID}.crl") so both DASHBOARD_INTERNAL and CA_UUID are treated as single tokens and safe from expansion issues; ensure you apply quoting consistently wherever this URL is constructed.
🤖 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 @.github/workflows/ci.yml:
- Around line 27-28: CI installs shfmt pinned to v3.10.0 via the curl command
URL and then chmods it; update this to use the newer upstream release (v3.13.1)
or explicitly keep v3.10.0 for reproducibility. Replace the download URL string
"https://github.com/mvdan/sh/releases/download/v3.10.0/shfmt_v3.10.0_linux_amd64"
used in the curl step and the corresponding chmod step to reference v3.13.1 if
you want the latest, and if bumping also update any related version checks or
hashes used elsewhere in the workflow.
In `@images/common/utils.sh`:
- Line 100: Change the string comparison in the shell conditional that checks
NGINX_HTTP_ALLOW to use the POSIX-compliant single equals operator; locate the
if statement that reads if [ "$NGINX_HTTP_ALLOW" == "True" ]; then and replace
the == comparison with = so the test becomes if [ "$NGINX_HTTP_ALLOW" = "True"
]; then to ensure the script remains compatible with /bin/sh.
- Around line 238-245: In openvpn_config_download, quote the URL arguments
passed to curl to prevent word-splitting (e.g. use
"${API_INTERNAL}/controller/vpn/download-config/${UUID}/?key=${KEY}" and
similarly for the checksum URL) and make the file globs safe when changing
permissions by using a non-option-prefixed path (e.g. chmod 600 ./*.pem or chmod
-- 600 *.pem) so filenames starting with '-' are not treated as options.
- Around line 233-236: Split declaration and command substitution so curl and
cat exit codes are preserved: first run curl --silent --insecure
"${API_INTERNAL}/controller/vpn/checksum/${UUID}/?key=${KEY}" > tmpfile and
check its exit status before exporting OFILE; likewise read NFILE with
NFILE="$(cat checksum)" and check that cat succeeded. Quote all variable
expansions (e.g. "${API_INTERNAL}", "${UUID}", "${KEY}") and only export OFILE
and NFILE after their commands complete successfully.
---
Outside diff comments:
In `@images/common/utils.sh`:
- Around line 247-261: The curl URL in crl_download uses unquoted variables
which can cause word splitting or globbing; update the command in crl_download
to quote the composed URL by surrounding the
${DASHBOARD_INTERNAL}/admin/pki/ca/x509/ca/${CA_UUID}.crl expression with double
quotes (e.g. "…${DASHBOARD_INTERNAL}/…${CA_UUID}.crl") so both
DASHBOARD_INTERNAL and CA_UUID are treated as single tokens and safe from
expansion issues; ensure you apply quoting consistently wherever this URL is
constructed.
🪄 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: 32c8e077-fa7e-4d46-ab62-fd777388def2
📒 Files selected for processing (9)
.github/workflows/bot-ci-failure.yml.github/workflows/ci.ymldeploy/auto-install.shdocs/developer/instructions.rstimages/common/init_command.shimages/common/openwisp/asgi.pyimages/common/utils.shimages/openwisp_dashboard/openvpn.jsonrun-qa-checks
📜 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 context used
📓 Path-based instructions (4)
**/*.{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:
images/common/openwisp/asgi.py
**/*.{sh,py,yml,yaml,dockerfile,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it
Files:
images/common/openwisp/asgi.pyimages/common/init_command.shdeploy/auto-install.shimages/common/utils.sh
{images/**,customization/**,deploy/**}/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
Avoid unnecessary blank lines inside functions or shell blocks
Files:
images/common/init_command.shdeploy/auto-install.shimages/common/utils.sh
{**/Dockerfile*,**/*.sh}
📄 CodeRabbit inference engine (AGENTS.md)
Watch for exposed secrets, unsafe defaults, insecure permissions, unsafe shell expansion, path traversal, and accidental public ports in Docker configurations and shell scripts
Files:
images/common/init_command.shdeploy/auto-install.shimages/common/utils.sh
🧠 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/init_command.shimages/common/utils.sh
🪛 Shellcheck (0.11.0)
images/common/utils.sh
[warning] 100-100: In POSIX sh, == in place of = is undefined.
(SC3014)
[warning] 233-233: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 233-234: Quote this to prevent word splitting.
(SC2046)
[info] 234-234: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 234-234: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 234-234: Double quote to prevent globbing and word splitting.
(SC2086)
[warning] 235-235: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 235-235: Quote this to prevent word splitting.
(SC2046)
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 240-240: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 242-242: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 244-244: Use ./glob or -- glob so names with dashes won't become options.
(SC2035)
[info] 249-249: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 249-249: Double quote to prevent globbing and word splitting.
(SC2086)
🔇 Additional comments (11)
.github/workflows/bot-ci-failure.yml (1)
15-15: LGTM!run-qa-checks (2)
7-7: LGTM!
9-13: LGTM!docs/developer/instructions.rst (1)
93-96: LGTM!images/common/utils.sh (3)
3-20: LGTM!
22-97: LGTM!
107-230: LGTM!deploy/auto-install.sh (1)
40-45: LGTM!images/common/init_command.sh (1)
69-70: LGTM!images/common/openwisp/asgi.py (1)
26-28: LGTM!images/openwisp_dashboard/openvpn.json (1)
14-23: LGTM!
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
images/common/utils.sh (1)
89-89:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReplace bash-specific
[[with POSIX[for sh compatibility.The double-bracket
[[ ... ]]syntax is a bash extension and will fail under/bin/shon systems whereshis not bash (e.g., dash, ash). Since this file declares#!/bin/shand the PR enforces shell script QA, use the POSIX single-bracket test.🔧 Proposed fix
- if [[ $? = "0" ]]; then + if [ $? = 0 ]; then🤖 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` at line 89, Replace the bash-only test in the condition "if [[ $? = "0" ]]; then" with a POSIX-compatible single-bracket test: use [ ... ] instead of [[ ... ]], perform a numeric comparison (use -eq) and quote the variable to avoid word-splitting; update the condition inside utils.sh accordingly so it works under /bin/sh.
🤖 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 240-246: In openvpn_config_download, add explicit error handling
around the two curl calls and subsequent tar/chmod steps: check each curl's exit
status (or use set -o pipefail and check) and abort with a clear error if they
fail; verify vpn.tar.gz exists and is non-empty before running tar and validate
the downloaded checksum file (e.g., compare checksum) and abort on mismatch;
ensure tar succeeds before running chmod and only chmod the expected *.pem files
after confirming they exist; on any failure clean up partial artifacts
(vpn.tar.gz, checksum) and return a non-zero status so callers can detect the
error.
- Around line 233-237: The current command substitutions for OFILE and NFILE
(OFILE=$(curl ... "${API_INTERNAL}/controller/vpn/checksum/${UUID}/?key=${KEY}")
and NFILE=$(cat checksum)) do not check exit status and can produce empty or
invalid values; update the block to capture curl and cat exit codes, validate
that OFILE and NFILE are non-empty and that curl returned 0 (or HTTP success),
and on failure log an error via the script's logger/echo and exit with a
non-zero status; specifically, after the curl invocation that sets OFILE and
after the cat checksum that sets NFILE, check their exit statuses ($? or
conditional checks) and handle missing/empty values by printing a clear error
including UUID/KEY and returning a failure code so downstream comparisons are
not performed with invalid data.
---
Outside diff comments:
In `@images/common/utils.sh`:
- Line 89: Replace the bash-only test in the condition "if [[ $? = "0" ]]; then"
with a POSIX-compatible single-bracket test: use [ ... ] instead of [[ ... ]],
perform a numeric comparison (use -eq) and quote the variable to avoid
word-splitting; update the condition inside utils.sh accordingly so it works
under /bin/sh.
🪄 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: 002e27a0-4c75-47a5-a7ee-49852941d195
📒 Files selected for processing (2)
.github/workflows/ci.ymlimages/common/utils.sh
📜 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 context used
📓 Path-based instructions (3)
{images/**,customization/**,deploy/**}/*.sh
📄 CodeRabbit inference engine (AGENTS.md)
Avoid unnecessary blank lines inside functions or shell blocks
Files:
images/common/utils.sh
{**/Dockerfile*,**/*.sh}
📄 CodeRabbit inference engine (AGENTS.md)
Watch for exposed secrets, unsafe defaults, insecure permissions, unsafe shell expansion, path traversal, and accidental public ports in Docker configurations and shell scripts
Files:
images/common/utils.sh
**/*.{sh,py,yml,yaml,dockerfile,Dockerfile}
📄 CodeRabbit inference engine (AGENTS.md)
Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it
Files:
images/common/utils.sh
🧠 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 (2)
.github/workflows/ci.yml (1)
27-28: LGTM!Also applies to: 35-35
images/common/utils.sh (1)
233-234: Reworkcurl --insecureusage inimages/common/utils.sh
images/common/utils.shusescurl --insecurefor internal controller/admin calls (checksum: 233-234, download-config: 242-243, CRL: 250-251). The file doesn’t show any--cacert/--capath(or other) approach to keep TLS verification on, so the security rationale isn’t clear.Confirm these endpoints are HTTPS with a self-signed cert only, and replace
--insecurewith a trusted internal CA (e.g.,--cacertpointing at the mounted CA). If these calls are plain HTTP, remove--insecure.
1 participant
Checklist
Reference to Existing Issue
Not needed as it's just a small QA improvement.
Description of Changes
Some formats were not QA checked: bash scripts, YAML, md and JSON files. This patch fixes this.