Fix permissions on Synology - #1268
Conversation
WalkthroughThe Dockerfile's READ_WRITE_FOLDERS set is augmented to include SYSTEM_NGINX_CONFIG, adding the nginx configuration directory to the container's writable paths. No logic or error handling changes; control flow remains unchanged. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Dockerfile (1)
175-189: Permission hierarchy design is sound but relies on "others" traversal.The hardened stage applies permissions in the correct order: read-only folders are set first (line 178–180), then read-write folders override with
chmod 700(lines 181–184). SinceSYSTEM_SERVICES_CONFIG(inREAD_ONLY_FOLDERS) becomes mode 005, andSYSTEM_NGINX_CONFIG(inREAD_WRITE_FOLDERS) becomes 700, thenetalertxuser must rely on "others" having execute permission on the parent to traverse into the child. This works because thenetalertxuser is neither owner nor group of the parent, placing it in the "others" category with r-x.Note: Lines 181 and 187–189 both apply
install -dtoREAD_WRITE_FOLDERS. This duplication appears intentional for idempotency but is worth documenting if it's not already covered elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1235
File: .devcontainer/scripts/setup.sh:146-148
Timestamp: 2025-10-26T17:09:18.621Z
Learning: In `.devcontainer/scripts/setup.sh` and other devcontainer setup scripts for NetAlertX, chmod 666 on /var/run/docker.sock is acceptable because devcontainer environments are single-user development contexts where convenience can take priority over strict permission hardening.
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
📚 Learning: 2025-09-20T14:09:29.159Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/scripts/setup.sh:103-116
Timestamp: 2025-09-20T14:09:29.159Z
Learning: In NetAlertX devcontainer setup, the netalertx user has write permissions to /var/log/nginx/ directory as it's explicitly chowned to netalertx:www-data in the Dockerfile, so setup.sh can write to nginx log files without sudo.
Applied to files:
Dockerfile
📚 Learning: 2025-09-20T03:01:19.912Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1184
File: .devcontainer/Dockerfile:18-19
Timestamp: 2025-09-20T03:01:19.912Z
Learning: In the NetAlertX repository, .devcontainer/Dockerfile is auto-generated and should not be reviewed directly. Review comments about dependencies and build steps should be directed at the root Dockerfile where the actual source commands are located.
Applied to files:
Dockerfile
📚 Learning: 2025-10-19T01:40:57.095Z
Learnt from: adamoutler
Repo: jokob-sk/NetAlertX PR: 1230
File: .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template:1-2
Timestamp: 2025-10-19T01:40:57.095Z
Learning: In the NetAlertX repository, .devcontainer/resources/devcontainer-overlay/services/config/nginx/netalertx.conf.template is an auto-generated file that is intentionally committed to source control. It cannot be regenerated automatically outside the devcontainer environment and is required for the devcontainer to start, creating a bootstrap dependency.
Applied to files:
Dockerfile
🔇 Additional comments (1)
Dockerfile (1)
88-90: The implementation is correct; no issues found.The change adds
${SYSTEM_NGINX_CONFIG}(/services/config/nginx) toREAD_WRITE_FOLDERS. This is intentional and correct:
SYSTEM_SERVICES_ACTIVE_CONFIG(/services/config/nginx/conf.active) is a child ofSYSTEM_NGINX_CONFIG- The recursive
findoperations at lines 180 and 184 automatically apply permissions to all subdirectories- Permission model is sound: parent directory (/services/config) retains 005 (r-x for others), enabling netalertx to traverse into child directories that have 700 (rwx for netalertx)
- Adding the parent directory is more robust than adding only the child directory
No verification needed; the permission hierarchy functions correctly.
This PR introduces a necessary workaround for persistent issues encountered on Docker environments that do not correctly honor the
tmpfsmodeparameter (e.g., specific older Linux kernels and Docker versions, such as those used in Synology DSM).These host limitations prevent the proper initialization of the
/services/config/nginx/conf.activedirectory, leading to:mvoperation required to place the active Nginx configuration, resulting in a startup error (mv: can't stat ... Permission denied).The fix is a defensive measure to ensure that this volatile directory is explicitly captured and handled by the existing hardening logic designed for writable paths.
Implementation:
The variable
SYSTEM_SERVICES_ACTIVE_CONFIGis formally added to theREAD_WRITE_FOLDERSlist in the Runner Stage.This modification forces the Hardened Stage's final permission sweep to include the Nginx configuration path in its explicit
chownandchmod 700application. This ensures that even if the host environment'stmpfsmounting fails to set the correct permissions, the container build process overrides it, granting thenetalertxuser the necessary write access for service startup on affected platforms.Summary by CodeRabbit