init-user: Safeguarded linking directories and files #922
Conversation
…lag to avoid link-loops. Catching linking of .gitignore and .ssh via a warning, so that s6 does does not crash (and with it, the container)
WalkthroughThe script has been updated to modify symbolic link creation. The commands now use Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
vscode/rootfs/etc/s6-overlay/s6-rc.d/init-user/run (1)
34-34: Consider additional security checks for .ssh directory.While converting the error to a warning helps prevent container crashes, the .ssh directory requires strict permissions for security. Consider adding permission verification after linking.
-ln -sn "${SSH_USER_PATH}" ~/.ssh || bashio::log.warning "Failed linking .ssh" +ln -sn "${SSH_USER_PATH}" ~/.ssh || bashio::log.warning "Failed linking .ssh" +if [[ -L ~/.ssh ]] && [[ -d ~/.ssh ]]; then + [[ "$(stat -c %a ~/.ssh)" == "700" ]] || { + chmod 700 ~/.ssh + bashio::log.warning "Fixed .ssh directory permissions" + } +fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
vscode/rootfs/etc/s6-overlay/s6-rc.d/init-user/run(3 hunks)
🔇 Additional comments (3)
vscode/rootfs/etc/s6-overlay/s6-rc.d/init-user/run (3)
14-17: LGTM! Good use of--no-dereferenceflag.The addition of
-nflag prevents the creation of deep links when the target is already a symbolic link, which is a good practice for container deployments.
20-23: LGTM! Consistent use of--no-dereferenceflag.The changes maintain consistency with the new linking approach while preserving backward compatibility.
44-45: LGTM! Appropriate use of force flag for ZSH history.The use of
-fflag and strict error handling is appropriate for the ZSH history file as it's critical for shell functionality.
| @@ -58,7 +58,7 @@ if ! bashio::fs.file_exists "${GIT_USER_PATH}/.gitconfig"; then | |||
| touch "${GIT_USER_PATH}/.gitconfig" \ | |||
| || bashio::exit.nok 'Failed to create .gitconfig' | |||
| fi | |||
| ln -s "${GIT_USER_PATH}/.gitconfig" ~/.gitconfig | |||
| ln -s "${GIT_USER_PATH}/.gitconfig" ~/.gitconfig || bashio::log.warning "Failed linking .gitconfig" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing --no-dereference flag for consistency.
The -n flag is missing in this ln command, making it inconsistent with other link commands in the script.
-ln -s "${GIT_USER_PATH}/.gitconfig" ~/.gitconfig || bashio::log.warning "Failed linking .gitconfig"
+ln -sn "${GIT_USER_PATH}/.gitconfig" ~/.gitconfig || bashio::log.warning "Failed linking .gitconfig"📝 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.
| ln -s "${GIT_USER_PATH}/.gitconfig" ~/.gitconfig || bashio::log.warning "Failed linking .gitconfig" | |
| ln -sn "${GIT_USER_PATH}/.gitconfig" ~/.gitconfig || bashio::log.warning "Failed linking .gitconfig" |
Proposed Changes
This PR adds the
--no-dereference(-n) flag for linking folders in order to avoid linking too deep as a good practice.It also convers linking errors of .ssh and .gitconfig to warnings via bashio to avoid s6 to crash the container for certain container deployment setups [1].
I observed that restarting the container (when deployed standalone [1]), the
init-userscript succeeds on the first container start without any problems. Another container restart however re-ran the init-user script, which caused two issues:ln -s /config ${HOME}/configcreated/config/configif${HOME}/configalready existed.init-usertried to recreated existing links, that were not guarded via||likeln -s src target || bashio::log.warning "The warning message"that absorbed the non-zero return code oflnon failues (.gitignoreand.ssh).However (I hope this does not get too quickly discared as a wont fix):
For container-addons spawned by the HA supervisor, these issues might not appear when the container is deployed as it is intended via HA and the supervisor.
However, standalone deployments [1] will cause this issue. Since my proposed changes are just good practice, I hope you consider to add my minute changes.
[1] Standalone deployment for either testing, debugging etc or in my case: manually integrating the container in an already existing big docker stack without using the supervisor
Summary by CodeRabbit