Skip to content

fix(install): re-run update path trips git dubious-ownership after the taos chown #768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jaylfc merged 2 commits into from
Jun 11, 2026
Merged

fix(install): re-run update path trips git dubious-ownership after the taos chown #768

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
43b258a
fix(install): scope a safe.directory exception for the re-run update …
jaylfc Jun 11, 2026
e0b166d
fix(install): drop to the repo owner for the re-run update instead of…
jaylfc Jun 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion scripts/install-server.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -980,7 +980,20 @@ if [[ ! -d "$INSTALL_DIR/.git" ]]; then
git clone --depth 1 --branch "$BRANCH" "$REPO" "$INSTALL_DIR"
else
log "updating existing checkout"
(cd "$INSTALL_DIR" && git fetch --depth 1 origin "$BRANCH" && git reset --hard "origin/$BRANCH")
# The repo is chowned to the 'taos' service user at the end of a system
# install, so a re-run (as root) trips git's dubious-ownership check.
# That check is guarding a real privilege-escalation path: running git as
# root inside a tree the unprivileged 'taos' user can write to would let a
# 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 "")"

@kilo-code-bot kilo-code-bot Bot Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kilo-code-bot kilo-code-bot Bot Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if [[ "$(id -u)" == "0" && -n "$_repo_owner" && "$_repo_owner" != "root" ]]; then

@kilo-code-bot kilo-code-bot Bot Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sudo -u "$_repo_owner" git -C "$INSTALL_DIR" fetch --depth 1 origin "$BRANCH" \

@kilo-code-bot kilo-code-bot Bot Jun 11, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

&& sudo -u "$_repo_owner" git -C "$INSTALL_DIR" reset --hard "origin/$BRANCH"
else
(cd "$INSTALL_DIR" && git fetch --depth 1 origin "$BRANCH" && git reset --hard "origin/$BRANCH")
fi
fi

cd "$INSTALL_DIR"
Expand Down
Loading