[change] Update theme customization paths and enhance collectstatic logic#579
[change] Update theme customization paths and enhance collectstatic logic#579
Conversation
…ogic Bug: Previously, when the OPENWISP_ADMIN_THEME_LINKS setting included any CSS or JavaScript file paths starting with /static/, the OpenWISP utilities logic would strip the /static/ prefix. As a result, Django couldn't recognize these custom static files, preventing them from being collected and served. Additionally, static files were previously mounted directly in the Nginx container, bypassing Django’s static collection workflow entirely. Fix: Mount custom theme assets inside the dashboard container and integrate them into Django’s staticfiles pipeline, ensuring they are processed by collectstatic and correctly served via Nginx.
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe diff exposes a project-level custom static directory (static_custom) to the dashboard container and mounts a dedicated customization/nginx directory into the nginx container. STATICFILES_DIRS was added to include static_custom. collectstatic.py gained get_dir_shasum, now computes pip and static_custom hashes, compares them against Redis-cached values, runs manage.py collectstatic (using --clear only when static_custom changed), and writes hashes back to Redis, then reloads uWSGI. Documentation and docker-compose mounts were updated, tests add setup/exec helpers and a Selenium test for custom CSS, CI adds an interactive tmate step and runs tests with sudo, and a minor Makefile/JSON tweak was applied. Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant FS as Filesystem
participant Compose as Docker Compose
participant Startup as collectstatic.py
participant Redis as Redis
participant Django as manage.py
participant uWSGI as uWSGI
participant Browser as Selenium
Dev->>FS: add/modify files under customization/theme (static_custom)
Compose->>Startup: run on container startup
Startup->>FS: get_dir_shasum(static_custom) -> current_static_hash
Startup->>Startup: compute current_pip_hash (pip freeze)
Startup->>Redis: GET pip_freeze_hash, static_custom_hash
note right of Startup: determine pip_changed, static_changed
alt pip_changed or static_changed
Startup->>Django: run collectstatic (add --clear if static_changed)
Django-->>Startup: collectstatic finished
Startup->>Redis: SET pip_freeze_hash, static_custom_hash
Startup->>uWSGI: uwsgi --reload
else no changes
Startup-->>Startup: skip collectstatic
end
Browser->>Compose: test via Selenium (load /admin/)
Browser->>Django: fetch admin page (serves collected static)
Browser-->>Dev: assert custom CSS applied
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/customization.rst`:
- Around line 190-191: Update the docs/user/customization.rst entry that
currently points to "customization/custom/maintenance.html": change the
documented maintenance file path to the actual mounted location
("customization/nginx/maintenance.html") and add a brief note that
docker-compose mounts ./customization/nginx into /opt/openwisp/public/custom so
placing maintenance.html there enables maintenance mode.
In `@images/common/collectstatic.py`:
- Around line 79-87: The call to run_collectstatic() should include the --clear
flag only when static_changed is true so stale files in STATIC_ROOT are removed;
modify the branch that checks pip_changed or static_changed to call
run_collectstatic("--clear") when static_changed is true and run_collectstatic()
without the flag otherwise, leaving the existing
redis_connection.set("pip_freeze_hash", current_pip_hash) and
redis_connection.set("static_custom_hash", current_static_hash) logic intact;
locate the conditional surrounding pip_changed/static_changed and the
run_collectstatic() invocation to implement this change.
🪄 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: efe678e1-ba46-4982-b0eb-1c57e626ba95
📒 Files selected for processing (4)
docker-compose.ymldocs/user/customization.rstimages/common/collectstatic.pyimages/common/openwisp/settings.py
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-06T11:55:13.672Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:85-85
Timestamp: 2026-01-06T11:55:13.672Z
Learning: In docker-compose.yml files, avoid leaving CELERY_SERVICE_NETWORK_MODE as an empty string; an empty value is ignored and Docker Compose falls back to default networking. Do not rely on setting CELERY_SERVICE_NETWORK_MODE to 'bridge' for celery services, as this will not affect their networking. If specific networking is required for celery services, define and attach explicit networks in the compose file and reference them on the celery services.
Applied to files:
docker-compose.yml
📚 Learning: 2026-01-06T11:56:48.600Z
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:85-85
Timestamp: 2026-01-06T11:56:48.600Z
Learning: In docker-openwisp projects, ensure CELERY_SERVICE_NETWORK_MODE is set to an empty string "" (which Docker Compose treats as unset/null). This allows containers to connect via the Compose default network with correct service name DNS resolution. Using "bridge" as the value disables service name resolution and breaks communication between celery, dashboard, postgres, and redis. Apply this guideline to docker-compose.yml files in the repository and any similar Compose files where CELERY services rely on service name DNS.
Applied to files:
docker-compose.yml
🔇 Additional comments (6)
docker-compose.yml (2)
32-32: Mounting theme assets intodashboardis the right integration point.This routes custom files through Django’s staticfiles pipeline instead of serving them out-of-band.
132-132: Splitting Nginx-only custom files intocustomization/nginxlooks good.Keeping
/opt/openwisp/public/customseparate fromstatic_customavoids mixing Nginx overrides with Django-managed assets.images/common/openwisp/settings.py (1)
287-287:STATICFILES_DIRSnow points at the custom theme root correctly.This makes
customization/theme/custom/...resolvable as/static/custom/...duringcollectstatic.images/common/collectstatic.py (1)
29-51: The directory hash helper is deterministic.Sorting
dirsandfilesbefore hashing keeps change detection stable across restarts.docs/user/customization.rst (2)
27-27: Addingcustomization/nginxto the bootstrap commands is useful.It matches the new bind mount and makes the directory layout explicit for first-time setups.
88-105: The revised theme workflow instructions match the new container layout.Pointing users at
customization/theme/custom/...and rerunningcollectstaticondashboardis consistent with the compose change.
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/customization.rst`:
- Around line 97-105: The note only mentions adding new files to
customization/theme but must also cover edits and deletions because the startup
only reacts to changes in static_custom; update the text to say "adding,
editing, or removing files in customization/theme (or any static_custom asset)
requires recreating the dashboard container or manually rerunning collectstatic
and reloading uWSGI" and keep the provided command (docker compose exec
dashboard bash -c "python collectstatic.py && uwsgi --reload uwsgi.pid") as the
manual alternative; ensure references to customization/theme, static_custom,
collectstatic.py, and uwsgi.pid are included so readers know exactly which
assets and commands are affected.
In `@images/common/collectstatic.py`:
- Around line 42-47: The directory hashing is ambiguous because
hasher.update(relpath.encode()) and then file bytes are concatenated with no
separator; update the logic in collectstatic.py (around where relpath is
computed and file bytes are fed into the hasher) to unambiguously frame
entries—for example, feed a length-prefixed relpath or append a fixed delimiter
(e.g., hasher.update(relpath.encode()); hasher.update(b'\0')) before streaming
file chunks in the block that opens fpath; ensure every hashed entry uses the
same framing so renames and different path splits produce different hashes.
In `@tests/config.json`:
- Line 13: The custom_css_filename in tests/config.json should be the
theme-relative filename only (no "customization/theme/" prefix) because
tests/runtests.py prepends the host-side theme path when creating/serving the
file; update the "custom_css_filename" value to just "custom-2hjsdf734653.css"
(referencing the key custom_css_filename in tests/config.json and the test
helper logic in tests/runtests.py that injects /static/{custom_css_filename}) so
the test request and the file creation align.
In `@tests/runtests.py`:
- Around line 95-100: The test writes a static CSS string which allows stale
cached assets to pass; change the write to include a unique per-run token (e.g.,
uuid4() or timestamp) when writing to css_path (use the same token variable when
calling custom_css_file.write) and update any browser/assertion code that
currently expects "1" to assert that exact token is present; also apply the same
change to the other identical block referenced around lines 239-247 so both test
helpers and assertions use the same per-run token.
- Around line 85-90: The helper currently spawns subprocess.Popen(cmd_args,
**kwargs), captures output/error but ignores the process exit status, so modify
the block that uses subprocess.Popen, cmd, cmd_args, cmd.communicate(), and
cls.config["logs_file"] to check cmd.returncode (or use subprocess.run with
check=True) after communicate(); if the return code is non‑zero, append
stderr/stdout to the logs as now and then raise an exception (e.g., rethrow
subprocess.CalledProcessError or raise RuntimeError) so the test fails fast
instead of proceeding silently.
🪄 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: 91141680-53ad-4022-a495-dd5604e3023a
📒 Files selected for processing (5)
Makefiledocs/user/customization.rstimages/common/collectstatic.pytests/config.jsontests/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). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-02-17T16:59:39.027Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:39.027Z
Learning: In all Makefiles, prefer 'include' (not '-include') for critical config files like .env. If the file is missing, the build should fail explicitly rather than silently proceed with empty values. This ensures configurations are present and avoids dangerous silent failures. Consider adding a clear comment near the line and validation steps in CI to verify .env presence.
Applied to files:
Makefile
📚 Learning: 2026-02-17T17:08:12.144Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Applied to files:
Makefile
🧬 Code graph analysis (1)
tests/runtests.py (2)
tests/utils.py (1)
login(43-49)images/common/collectstatic.py (1)
main(65-96)
🔇 Additional comments (1)
Makefile (1)
5-5: Good fail-fast config include on Line 5.Using
include .env(instead of-include) is the right choice for a critical config file and prevents silent misconfiguration.Based on learnings: "In all Makefiles, prefer 'include' (not '-include') for critical config files like .env."
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/runtests.py (1)
251-259:⚠️ Potential issue | 🟡 MinorStrip whitespace from CSS property value before comparison.
getComputedStyle().getPropertyValue()may return values with leading/trailing whitespace depending on the browser. Using.strip()ensures the assertion is robust across different browsers.🐛 Proposed fix
value = self.web_driver.execute_script( "return getComputedStyle(document.body)" ".getPropertyValue('--openwisp-test');" ) - self.assertEqual(value, "1") + self.assertEqual(value.strip(), "1")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtests.py` around lines 251 - 259, The test test_custom_static_files_loaded can fail because getComputedStyle(...).getPropertyValue(...) may include surrounding whitespace; update the assertion to strip whitespace before comparing: after calling self.web_driver.execute_script(...) (the script that returns getComputedStyle(document.body).getPropertyValue('--openwisp-test')), call .strip() on the returned value (or apply .strip() inside the JS return) so the comparison self.assertEqual(value, "1") uses the trimmed string; modify the value handling in the test_custom_static_files_loaded test to normalize whitespace before the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/runtests.py`:
- Around line 67-95: The _execute_docker_compose_command function currently uses
map(str, (cmd.stdout, cmd.stderr)) which yields b'...' style strings when
use_text_mode is False; change the handling so that when use_text_mode is True
you use cmd.stdout/cmd.stderr directly, otherwise decode the bytes using
.decode('utf-8', errors='replace') (or similar) to produce proper text; ensure
you update the variables output and error accordingly before writing to
cls.config["logs_file"] and before returning, referencing the
_execute_docker_compose_command method and the cmd.stdout/cmd.stderr variables.
---
Duplicate comments:
In `@tests/runtests.py`:
- Around line 251-259: The test test_custom_static_files_loaded can fail because
getComputedStyle(...).getPropertyValue(...) may include surrounding whitespace;
update the assertion to strip whitespace before comparing: after calling
self.web_driver.execute_script(...) (the script that returns
getComputedStyle(document.body).getPropertyValue('--openwisp-test')), call
.strip() on the returned value (or apply .strip() inside the JS return) so the
comparison self.assertEqual(value, "1") uses the trimmed string; modify the
value handling in the test_custom_static_files_loaded test to normalize
whitespace before the assertion.
🪄 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: d52ec746-333e-4557-a9e8-0585f0d3cc45
📒 Files selected for processing (4)
.github/workflows/ci.ymldocs/user/customization.rstimages/common/collectstatic.pytests/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). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧬 Code graph analysis (1)
tests/runtests.py (2)
tests/utils.py (1)
login(43-49)images/common/collectstatic.py (1)
main(69-100)
🔇 Additional comments (9)
docs/user/customization.rst (2)
86-105: LGTM!The documentation correctly describes the updated workflow for custom CSS/JS:
- The path
customization/theme/custom/css/custom-theme.cssaligns with the mount tostatic_customin the dashboard container.- The note properly covers adding, updating, or removing files.
- The alternative command using
python collectstatic.py && uwsgi --reload uwsgi.pidmatches the implementation inimages/common/collectstatic.py.
187-200: LGTM!The maintenance mode section correctly documents that
customization/nginx/maintenance.htmlis the path to enable maintenance mode, which aligns with the docker-compose mount of./customization/nginxinto the nginx container's public custom directory..github/workflows/ci.yml (1)
81-81: LGTM!The CI test command correctly runs tests with
sudo(consistent with the setup that usessudo pip3 install) and properly stops containers afterward.images/common/collectstatic.py (3)
29-55: LGTM!The
get_dir_shasumfunction correctly implements deterministic directory hashing:
- Sorted traversal ensures consistent ordering across runs.
- Length-prefixed relative paths prevent hash collisions from path/content concatenation ambiguity.
- Graceful handling of missing directories and unreadable files.
58-66: LGTM!The
run_collectstaticfunction correctly conditionally appends--clearbased on theclearparameter, ensuring stale files are removed only whenstatic_customchanges.
84-95: LGTM!The logic correctly:
- Runs
collectstaticwhen either pip dependencies or static files change.- Uses
--clearonly when static files changed (to remove stale assets).- Gracefully handles Redis write failures without crashing startup since
collectstaticalready completed.tests/runtests.py (3)
97-131: LGTM!The
_setup_admin_theme_linksmethod correctly:
- Uses
grep -qto prevent duplicateOPENWISP_ADMIN_THEME_LINKSentries on re-runs.- Runs
collectstatic.pyand reloads uWSGI to apply changes.- The inline script is well-structured for container execution.
181-187: LGTM!Proper cleanup of the temporary CSS file in
tearDownClassensures test isolation between runs.
566-566: LGTM!Using
unittest.main(verbosity=2)is cleaner than manually constructing a test suite and provides better test output.
| @classmethod | ||
| def _execute_docker_compose_command(cls, cmd_args, use_text_mode=False): | ||
| """Execute a docker compose command and log output. | ||
|
|
||
| Args: | ||
| cmd_args: List of command arguments for subprocess.Popen | ||
| use_text_mode: If True, use text mode for subprocess output | ||
|
|
||
| Returns: | ||
| Tuple of (output, error) from command execution | ||
| """ | ||
| kwargs = { | ||
| "stdout": subprocess.PIPE, | ||
| "stderr": subprocess.PIPE, | ||
| "cwd": cls.root_location, | ||
| } | ||
| if use_text_mode: | ||
| kwargs["text"] = True | ||
| cmd = subprocess.run(cmd_args, check=False, **kwargs) | ||
| output, error = map(str, (cmd.stdout, cmd.stderr)) | ||
| with open(cls.config["logs_file"], "a") as logs_file: | ||
| logs_file.write(output) | ||
| logs_file.write(error) | ||
| if cmd.returncode != 0: | ||
| raise RuntimeError( | ||
| f"docker compose command failed " | ||
| f"({cmd.returncode}): {' '.join(cmd_args)}" | ||
| ) | ||
| return output, error |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider the bytes-to-string conversion for log output.
When use_text_mode=False, cmd.stdout and cmd.stderr are bytes objects. Using map(str, ...) on bytes produces strings like "b'actual content'" rather than decoded text, which makes the logs harder to read.
♻️ Proposed improvement
- cmd = subprocess.run(cmd_args, check=False, **kwargs)
- output, error = map(str, (cmd.stdout, cmd.stderr))
+ cmd = subprocess.run(cmd_args, check=False, **kwargs)
+ if use_text_mode:
+ output, error = cmd.stdout, cmd.stderr
+ else:
+ output = cmd.stdout.decode("utf-8", errors="replace") if cmd.stdout else ""
+ error = cmd.stderr.decode("utf-8", errors="replace") if cmd.stderr else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/runtests.py` around lines 67 - 95, The _execute_docker_compose_command
function currently uses map(str, (cmd.stdout, cmd.stderr)) which yields b'...'
style strings when use_text_mode is False; change the handling so that when
use_text_mode is True you use cmd.stdout/cmd.stderr directly, otherwise decode
the bytes using .decode('utf-8', errors='replace') (or similar) to produce
proper text; ensure you update the variables output and error accordingly before
writing to cls.config["logs_file"] and before returning, referencing the
_execute_docker_compose_command method and the cmd.stdout/cmd.stderr variables.
pandafy
force-pushed
the
fix-theme-customizations
branch
from
ec161a5 to
fef33a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Line 84: The command chain currently uses && so `make stop` is skipped if
tests fail and also drops privileges (no sudo on stop); change the command so
cleanup always runs and uses the same privilege level — e.g., run both targets
with sudo and separate with a semicolon or a shell construct that guarantees
execution (replace `... && sudo make develop-pythontests && make stop` with
something like `cd /opt/openwisp/docker-openwisp && sudo make
develop-pythontests; sudo make stop` or an equivalent `bash -c 'cd ... && sudo
make develop-pythontests; sudo make stop'`) so `make stop` always executes even
on test failure.
- Around line 71-73: The tmate step ("Setup tmate session" using
mxschmitt/action-tmate@v3) must be made opt-in and hardened: gate the step so it
only runs for manual workflow dispatch (e.g., check github.event_name ==
'workflow_dispatch' in the step's if condition) and add the action input to
restrict keys (set limit-access-to-actor: 'true' via the action's with: inputs)
so the session only allows the actor's registered SSH key; do not leave the step
unconditional in regular CI runs.
🪄 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: ec3542f9-c5c6-41ae-8ba1-b223b0d14bda
📒 Files selected for processing (1)
.github/workflows/ci.yml
.github/workflows/ci.yml
Outdated
| - name: Setup tmate session | ||
| uses: mxschmitt/action-tmate@v3 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For mxschmitt/action-tmate@v3, what are the default values for "detached" and access controls, and what are the recommended security practices for enabling it in CI on public repositories?
💡 Result:
Defaults (mxschmitt/action-tmate@v3)
detached: defaultfalse(the step blocks until you end the session). [1][2]- Access controls:
limit-access-to-actor: defaultauto. Inautomode, the action restricts SSH access to the workflow actor’s GitHub-profile public SSH keys if they have any; otherwise the session is open to “anybody who has the connection string.” [1][3]
Recommended security practices for CI on public repositories
- Do not enable tmate on untrusted triggers (e.g., PRs from forks / untrusted code paths). Prefer manual enablement (e.g.,
workflow_dispatchinput) and/or tightif:conditions (only on failure, only for maintainers, only on specific branches). [2][4] - Force authentication: set
limit-access-to-actor: trueso the session requires the actor to have a public SSH key (instead of falling back to open access). [3] - Minimize blast radius of the workflow:
- Set least-privilege
GITHUB_TOKENpermissions (ideally read-only unless required). [5] - Avoid mixing tmate with steps that expose secrets; assume anything on the runner could be accessed once an interactive shell is available. [4]
- Set least-privilege
- Pin action versions (use a full commit SHA, not just
@v3) to reduce supply-chain risk. [6] - Avoid privileged triggers like
pull_request_targetfor anything that could run untrusted PR code (especially with interactive access). [6]
Sources:
[1] mxschmitt/action-tmate action.yml (raw) (raw.githubusercontent.com)
[2] mxschmitt/action-tmate README – Detached mode (github.com)
[3] mxschmitt/action-tmate README – “Use registered public SSH key(s)” / limit-access-to-actor (github.com)
[4] GitHub Docs – securing your use of GitHub Actions (docs.github.com)
[5] GitHub Changelog – controlling GITHUB_TOKEN permissions; fork PRs get read-only token (github.blog)
[6] GitHub Well-Architected – Actions security (pinning; caution with pull_request_target) (wellarchitected.github.com)
Citations:
- 1: https://raw.githubusercontent.com/mxschmitt/action-tmate/master/action.yml
- 2: https://github.com/mxschmitt/action-tmate
- 3: https://github.com/mxschmitt/action-tmate
- 4: https://docs.github.com/en/enterprise-cloud%40latest/actions/how-tos/security-for-github-actions/security-guides/using-githubs-security-features-to-secure-your-use-of-github-actions?utm_source=openai
- 5: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token?utm_source=openai
- 6: https://wellarchitected.github.com/library/application-security/recommendations/actions-security/?utm_source=openai
Gate tmate to explicit debug-only runs.
Line 71 introduces an unconditional remote debug shell. By default, limit-access-to-actor is set to auto, which restricts SSH access to the actor's registered public SSH keys only if they exist—otherwise the session is open to anyone with the connection string. This opens the CI environment to unauthorized access and increases attack surface on a privileged runner context. Make this step opt-in, gated to a manual trigger, and hardened.
Suggested fix
- - name: Setup tmate session
- uses: mxschmitt/action-tmate@v3
+ - name: Setup tmate session
+ if: ${{ failure() && github.event_name == 'workflow_dispatch' && vars.ENABLE_TMATE == '1' }}
+ uses: mxschmitt/action-tmate@v3
+ with:
+ detached: true
+ limit-access-to-actor: true📝 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.
| - name: Setup tmate session | |
| uses: mxschmitt/action-tmate@v3 | |
| - name: Setup tmate session | |
| if: ${{ failure() && github.event_name == 'workflow_dispatch' && vars.ENABLE_TMATE == '1' }} | |
| uses: mxschmitt/action-tmate@v3 | |
| with: | |
| detached: true | |
| limit-access-to-actor: true |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 71 - 73, The tmate step ("Setup tmate
session" using mxschmitt/action-tmate@v3) must be made opt-in and hardened: gate
the step so it only runs for manual workflow dispatch (e.g., check
github.event_name == 'workflow_dispatch' in the step's if condition) and add the
action input to restrict keys (set limit-access-to-actor: 'true' via the
action's with: inputs) so the session only allows the actor's registered SSH
key; do not leave the step unconditional in regular CI runs.
.github/workflows/ci.yml
Outdated
| # and environment variables remain intact, it is essential to | ||
| # execute the test from this directory. | ||
| command: cd /opt/openwisp/docker-openwisp && make develop-pythontests && make stop | ||
| command: cd /opt/openwisp/docker-openwisp && sudo make develop-pythontests && make stop |
There was a problem hiding this comment.
Ensure cleanup always runs and keep privilege level consistent.
Line 84 uses &&, so make stop is skipped if tests fail, and it may also fail due to running without sudo after sudo make .... This can leave containers running and contaminate retries.
Suggested fix
- command: cd /opt/openwisp/docker-openwisp && sudo make develop-pythontests && make stop
+ command: |
+ cd /opt/openwisp/docker-openwisp
+ sudo make develop-pythontests
+ test_exit=$?
+ sudo make stop
+ exit $test_exit📝 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.
| command: cd /opt/openwisp/docker-openwisp && sudo make develop-pythontests && make stop | |
| command: | | |
| cd /opt/openwisp/docker-openwisp | |
| sudo make develop-pythontests | |
| test_exit=$? | |
| sudo make stop | |
| exit $test_exit |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml at line 84, The command chain currently uses && so
`make stop` is skipped if tests fail and also drops privileges (no sudo on
stop); change the command so cleanup always runs and uses the same privilege
level — e.g., run both targets with sudo and separate with a semicolon or a
shell construct that guarantees execution (replace `... && sudo make
develop-pythontests && make stop` with something like `cd
/opt/openwisp/docker-openwisp && sudo make develop-pythontests; sudo make stop`
or an equivalent `bash -c 'cd ... && sudo make develop-pythontests; sudo make
stop'`) so `make stop` always executes even on test failure.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
84-84:⚠️ Potential issue | 🟠 MajorEnsure teardown always runs and use consistent privileges.
Line 84 chains with
&&, somake stopis skipped if tests fail, and teardown dropssudo. This can leave containers running.Suggested fix
- command: cd /opt/openwisp/docker-openwisp && sudo make develop-pythontests && make stop + command: | + cd /opt/openwisp/docker-openwisp + sudo make develop-pythontests + test_exit=$? + sudo make stop + exit $test_exit#!/bin/bash # Verify command guarantees cleanup and keeps privilege level consistent rg -n -C3 'command:|develop-pythontests|make stop' .github/workflows/ci.yml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml at line 84, The current single-line command "cd /opt/openwisp/docker-openwisp && sudo make develop-pythontests && make stop" skips teardown if tests fail and drops sudo for stop; change it so the test step runs with sudo, capture its exit status, always run "make stop" with sudo regardless of test outcome, and finally exit with the original test exit code (i.e., replace the chained && form with a shell sequence that runs "sudo make develop-pythontests", stores its exit code, always runs "sudo make stop", then exits with the stored code).
71-73:⚠️ Potential issue | 🟠 MajorMake tmate debug access explicit, opt-in, and restricted.
Line 71 runs an interactive shell on normal CI paths with no guard. Keep this debug-only and restrict SSH access explicitly.
Suggested fix
- - name: Debugging with tmate - uses: mxschmitt/action-tmate@v3.23 + - name: Debugging with tmate + if: ${{ failure() && github.event_name == 'workflow_dispatch' && vars.ENABLE_TMATE == '1' }} + uses: mxschmitt/action-tmate@v3.23 + with: + detached: true + limit-access-to-actor: true#!/bin/bash # Verify tmate step is gated and hardened rg -n -C3 'Debugging with tmate|mxschmitt/action-tmate|workflow_dispatch|ENABLE_TMATE|limit-access-to-actor|detached' .github/workflows/ci.yml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 71 - 73, Make the tmate step ("Debugging with tmate" using mxschmitt/action-tmate@v3.23) opt-in and restricted by gating it behind an explicit manual trigger and an allow-list/env flag: require workflow_dispatch or an input like ENABLE_TMATE to be true before the step runs, add an `if` conditional on that input (e.g., only run when github.event_name == 'workflow_dispatch' and inputs.ENABLE_TMATE == 'true'), and harden the action inputs by enabling the action's actor-restriction option (the action's "limit access to actor" / similar input) and disabling any detached/always-open behaviour so it cannot run on normal CI paths. Ensure these changes are applied to the "Debugging with tmate" step and document the new workflow input so users must explicitly opt in.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Line 84: The current single-line command "cd /opt/openwisp/docker-openwisp &&
sudo make develop-pythontests && make stop" skips teardown if tests fail and
drops sudo for stop; change it so the test step runs with sudo, capture its exit
status, always run "make stop" with sudo regardless of test outcome, and finally
exit with the original test exit code (i.e., replace the chained && form with a
shell sequence that runs "sudo make develop-pythontests", stores its exit code,
always runs "sudo make stop", then exits with the stored code).
- Around line 71-73: Make the tmate step ("Debugging with tmate" using
mxschmitt/action-tmate@v3.23) opt-in and restricted by gating it behind an
explicit manual trigger and an allow-list/env flag: require workflow_dispatch or
an input like ENABLE_TMATE to be true before the step runs, add an `if`
conditional on that input (e.g., only run when github.event_name ==
'workflow_dispatch' and inputs.ENABLE_TMATE == 'true'), and harden the action
inputs by enabling the action's actor-restriction option (the action's "limit
access to actor" / similar input) and disabling any detached/always-open
behaviour so it cannot run on normal CI paths. Ensure these changes are applied
to the "Debugging with tmate" step and document the new workflow input so users
must explicitly opt in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4b336e50-67a6-47cb-96ea-7bb4a594c99c
📒 Files selected for processing (1)
.github/workflows/ci.yml
pandafy
force-pushed
the
fix-theme-customizations
branch
from
8cefe62 to
0a28ec9
Compare
pandafy
force-pushed
the
fix-theme-customizations
branch
from
cf50319 to
7012f62
Compare
pandafy
force-pushed
the
fix-theme-customizations
branch
from
7012f62 to
b59d717
Compare
1 participant
Checklist
Description of Changes
Bug:
Previously, when the OPENWISP_ADMIN_THEME_LINKS setting included any CSS or JavaScript file paths starting with /static/, the OpenWISP utilities logic would strip the /static/ prefix. As a result, Django couldn't recognize these custom static files, preventing them from being collected and served. Additionally, static files were previously mounted directly in the Nginx container, bypassing Django’s static collection workflow entirely.
Fix:
Mount custom theme assets inside the dashboard container and integrate them into Django’s staticfiles pipeline, ensuring they are processed by collectstatic and correctly served via Nginx.