fix(installer): normalise tree ownership before re-run update (unblocks re-runs after a partial install) - #1839
Conversation
A re-run of install-server.sh over an existing checkout drops to the repo-owning user (the 'taos' service user) for the git fetch + reset, to avoid running git as root inside a user-writable tree. But it only reads the TOP-LEVEL dir owner. If a prior install was interrupted mid-chown (or a root step wrote a few paths back), the tree has MIXED ownership: the owning user then cannot unlink the still-root-owned paths, so the reset fails with 'unable to unlink old ...: Permission denied' -> 'Could not reset index file to revision origin/master', bricking every subsequent re-run. Normalise ownership to the owning user (chown -R, run by root) right before the update so the reset can rewrite the whole tree. Safe: root does the chown and git still runs unprivileged. Reported on #2 (fresh Orange Pi 5 Plus, retry after a partial first run). Failure class reproduced locally: an unwritable path in the tree yields the identical unlink-EACCES; normalising the tree makes the reset apply cleanly.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThe installer’s root re-run path now recursively changes ownership of a non-root-owned checkout to its detected owner before running ChangesInstaller ownership handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # "Could not reset index file"), bricking every re-run. Normalise | ||
| # ownership to the owning user first so the reset can rewrite the whole | ||
| # tree. Safe: root performs the chown and git still runs unprivileged. | ||
| chown -R "$_repo_owner" "$INSTALL_DIR" |
There was a problem hiding this comment.
WARNING: chown -R dereferences symlinks by default (GNU chown without -h/--no-dereference).
The surrounding code is explicitly defending against a hostile-writable tree (git's dubious-ownership / privilege-escalation guard). If an attacker can plant a symlink inside $INSTALL_DIR, a root-run chown -R will follow it and walk outside the tree, handing ownership of arbitrary files to the unprivileged $_repo_owner. That partially undermines the very isolation this block is trying to preserve. Use chown -Rh "$_repo_owner" "$INSTALL_DIR" (or --no-dereference) so symlink targets are not traversed.
| chown -R "$_repo_owner" "$INSTALL_DIR" | |
| chown -Rh "$_repo_owner" "$INSTALL_DIR" |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # "Could not reset index file"), bricking every re-run. Normalise | ||
| # ownership to the owning user first so the reset can rewrite the whole | ||
| # tree. Safe: root performs the chown and git still runs unprivileged. | ||
| chown -R "$_repo_owner" "$INSTALL_DIR" |
There was a problem hiding this comment.
SUGGESTION: This chown -R runs unguarded under set -euo pipefail (set at line 35). If it fails for any reason — a path vanished mid-run, EPERM on an immutable/locked file, or chown hitting a mount it can't traverse — the entire install aborts before the git fetch/reset even runs. Since this is a best-effort normalisation step added to recover from a degraded state, consider guarding it (e.g. chown -Rh ... || warn "ownership normalisation incomplete; continuing") so a chown hiccup doesn't turn a recoverable re-run into a hard failure.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 files)
Fix these issues in Kilo Cloud Reviewed by hy3:free · Input: 36.9K · Output: 2.6K · Cached: 118.8K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/install-server.sh (1)
1173-1173: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winNormalize group ownership and prevent option injection.
Consider appending a colon (
:) to$_repo_owner(assuming it holds only the username). This instructschownto also normalize the group to the user's primary login group, cleaning up any residualrootgroup ownership from the interrupted install, which could otherwise cause inconsistent permissions orsetgidinheritance issues.Additionally, use
--to prevent path option injection if$INSTALL_DIRever begins with a hyphen.🛠️ Proposed refactor
- chown -R "$_repo_owner" "$INSTALL_DIR" + chown -R -- "$_repo_owner:" "$INSTALL_DIR"Security Note (Privilege Escalation via Hardlinks):
The inline comment notes that runningchownasrootis safe here, but keep in mind that executingchown -Rasrooton a directory owned by an unprivileged user introduces a classic hardlink attack vector. If the underlying OS does not enforce hardlink protections (i.e.,fs.protected_hardlinks=0), a malicious user could hardlink a sensitive file (e.g.,/etc/shadow) into$INSTALL_DIR. The recursivechownwould then inadvertently transfer ownership of the original file to$_repo_owner. Since modern Linux kernels enableprotected_hardlinks=1by default, this risk is mitigated on most contemporary systems, but it remains an architectural consideration if this installer supports legacy or custom OS environments.🤖 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 `@scripts/install-server.sh` at line 1173, Update the recursive ownership command around chown to use "$_repo_owner:" so the repository owner’s primary group is normalized, and add "--" before "$INSTALL_DIR" to prevent option injection. Preserve the existing recursive ownership behavior and do not broaden the change to unrelated hardlink protections.
🤖 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.
Nitpick comments:
In `@scripts/install-server.sh`:
- Line 1173: Update the recursive ownership command around chown to use
"$_repo_owner:" so the repository owner’s primary group is normalized, and add
"--" before "$INSTALL_DIR" to prevent option injection. Preserve the existing
recursive ownership behavior and do not broaden the change to unrelated hardlink
protections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: edccaa4b-f415-4a8b-bdcb-dd6128e7aaa0
📒 Files selected for processing (1)
scripts/install-server.sh
1 participant
Problem
Reported on #2 by a community tester on a fresh Orange Pi 5 Plus (Armbian trixie). A re-run of
install-server.shover an existing checkout dies with:Root cause
The update path deliberately drops to the repo-owning user (
taos) for thegit fetch+reset --hard, so git never runs as root inside a user-writable tree (a real privilege-escalation guard). But it decides the owner bystat-ing only the top-level$INSTALL_DIR.If a prior install was interrupted mid-
chown(or any root step wrote a few paths back as root), the tree ends up with mixed ownership. The owning user then cannot unlink the still-root-owned paths, soreset --hardfails and every subsequent re-run is bricked. This is easy to hit: first run hiccups (tight disk, incus init, Ctrl-C), second run cannot recover.Fix
Normalise ownership to the owning user with
chown -R(performed by root) immediately before the update, so the reset can rewrite the whole tree. git still runs unprivileged, so the security guard the original code added is preserved.Validation
bash -nclean.unable to unlink ...: Permission denied; normalising the tree makesreset --hardapply cleanly (correctly removing deleted paths).Summary by CodeRabbit