fix(install): re-run update path trips git dubious-ownership after the taos chown - #768
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe installer update step now detects the owner of $INSTALL_DIR and, when run as root against a non-root-owned checkout, performs ChangesOwner-aware repo update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
… overriding git ownership trust Running git as root inside a taos-owned tree (so a planted .git/config or hook could execute as root) is the privilege-escalation the dubious-ownership check guards against. Drop to the owning user for fetch/reset rather than bypassing the check; run directly when root-owned or not running as root.
|
Updated per a security review of the first commit: the original approach overrode git's safe.directory check, which would have left root running git inside a taos-writable tree (a planted .git/config or hook could execute as root). The dubious-ownership check exists to prevent exactly that. The update now drops to the repo's owning user (sudo -u) for fetch/reset, and only runs git directly when the tree is root-owned or the installer is not running as root (user-mode / macOS). |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| # planted .git/config or hook execute as root. So drop to the owning user | ||
| # for the update instead of overriding the check. When the tree is already | ||
| # root-owned, or we are not root (user-mode / macOS install), run directly. | ||
| _repo_owner="$(stat -c '%U' "$INSTALL_DIR" 2>/dev/null || stat -f '%Su' "$INSTALL_DIR" 2>/dev/null || echo "")" |
There was a problem hiding this comment.
WARNING: _repo_owner variable leaks to global scope
The variable _repo_owner is assigned at the top level without local declaration. While this script doesn't use functions for this section, global namespace pollution can cause subtle bugs if this variable name is used elsewhere. Consider using a more unique name (e.g., _taos_repo_owner) or wrapping this logic in a function with local.
| # for the update instead of overriding the check. When the tree is already | ||
| # root-owned, or we are not root (user-mode / macOS install), run directly. | ||
| _repo_owner="$(stat -c '%U' "$INSTALL_DIR" 2>/dev/null || stat -f '%Su' "$INSTALL_DIR" 2>/dev/null || echo "")" | ||
| if [[ "$(id -u)" == "0" && -n "$_repo_owner" && "$_repo_owner" != "root" ]]; then |
There was a problem hiding this comment.
WARNING: No validation that the owning user exists before sudo -u
If $INSTALL_DIR was manually chowned to a non-existent user (or a user that was deleted), the stat call succeeds but sudo -u "$_repo_owner" will fail with a confusing error like "sudo: unknown user: ...". The script will exit due to set -e, but the error message won't clearly indicate the root cause. Consider adding a check: id -u "$_repo_owner" >/dev/null 2>&1 before attempting the sudo.
| # planted .git/config or hook execute as root. So drop to the owning user | ||
| # for the update instead of overriding the check. When the tree is already | ||
| # root-owned, or we are not root (user-mode / macOS install), run directly. | ||
| _repo_owner="$(stat -c '%U' "$INSTALL_DIR" 2>/dev/null || stat -f '%Su' "$INSTALL_DIR" 2>/dev/null || echo "")" |
There was a problem hiding this comment.
SUGGESTION: Explicitly handle symlinks with stat -L
The stat command follows symlinks by default, but being explicit with -L (GNU) or relying on default behavior makes the intent clearer. Since $INSTALL_DIR could theoretically be a symlink (though unlikely in this script's flow), consider: stat -L -c '%U' ... for GNU stat. Note: BSD stat doesn't support -L flag the same way, so the current fallback chain handles it.
| # root-owned, or we are not root (user-mode / macOS install), run directly. | ||
| _repo_owner="$(stat -c '%U' "$INSTALL_DIR" 2>/dev/null || stat -f '%Su' "$INSTALL_DIR" 2>/dev/null || echo "")" | ||
| if [[ "$(id -u)" == "0" && -n "$_repo_owner" && "$_repo_owner" != "root" ]]; then | ||
| sudo -u "$_repo_owner" git -C "$INSTALL_DIR" fetch --depth 1 origin "$BRANCH" \ |
There was a problem hiding this comment.
SUGGESTION: Capture and surface sudo/git errors more clearly
If sudo -u fails (e.g., user doesn't exist, sudo not configured for passwordless), the script exits via set -e but the error message comes from sudo/git directly. Wrapping in a function or adding explicit error handling would improve debuggability. Example: sudo -u "$_repo_owner" git -C "$INSTALL_DIR" fetch ... || die "git fetch as $_repo_owner failed"
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Reviewed by nemotron-3-ultra-550b-a55b-20260604:free · 388,125 tokens |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
1 participant
Re-running the installer on an existing install fails at 'updating existing checkout' with git's dubious-ownership error: the repo is chowned to the taos service user at the end of every install, and the next run executes git fetch/reset as root inside it. Reported in #765, and it blocks exactly the re-run-with-sudo recovery path we point users at.
Fix scopes a safe.directory exception to the two update commands (no global git config changes). The post-install chown re-fixes ownership of anything the update creates.
Fixes #765.
Summary by CodeRabbit