Skip to content

feat: file permissions fix for multi-user environments #906

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

Open
ddtcorex wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: file permissions fix for multi-user environments #906

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 25 additions & 0 deletions commands/env.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,31 @@ then
fi
fi

## fix file permissions for multi-user environments
## this ensures www-data UID/GID matches the host user to prevent permission issues
if [[ "${WARDEN_PARAMS[0]}" == "up" ]] || [[ "${WARDEN_PARAMS[0]}" == "start" ]]; then
for container in php-fpm php-debug php-spx; do
# Check if container is running (exact match) and www-data UID doesn't match host UID

Copilot AI Jan 22, 2026

Copy link

Choose a reason for hiding this comment

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

The comment states "Check if container is running (exact match)" but the actual check using 'ps --services' only verifies that the service is defined in the docker-compose configuration, not that the container is actually running. This comment is misleading and should be corrected to accurately reflect what the code does, or the code should be updated to actually verify the container is running.

Suggested change
# Check if container is running (exact match) and www-data UID doesn't match host UID
# Check if container service exists (exact match) and www-data UID doesn't match host UID

Copilot uses AI. Check for mistakes.
CONTAINER_UID=$($WARDEN_BIN env exec "$container" id -u www-data 2>/dev/null || echo "")
if $WARDEN_BIN env ps --services 2>/dev/null | grep -q "^${container}$" \
&& [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then

Copilot AI Jan 22, 2026

Copy link

Choose a reason for hiding this comment

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

The check 'CONTAINER_UID != HOST_UID' only validates that UIDs differ, but doesn't validate that CONTAINER_UID is a valid numeric value. If the 'id -u www-data' command returns an unexpected value or error message (despite the '|| echo ""' fallback), this could cause unexpected behavior. Consider adding validation to ensure CONTAINER_UID contains only digits before comparison.

Suggested change
&& [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then
&& [[ -n "$CONTAINER_UID" ]] \
&& [[ "$CONTAINER_UID" =~ ^[0-9]+$ ]] \
&& [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then

Copilot uses AI. Check for mistakes.
Comment on lines +270 to +273

Copilot AI Jan 22, 2026

Copy link

Choose a reason for hiding this comment

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

The condition checks if the container service exists using 'ps --services', but then immediately attempts to exec into it to get the CONTAINER_UID. If the container is defined but not running yet (e.g., during 'up' before containers are fully started), the exec will fail silently and CONTAINER_UID will be empty, preventing the fix from running. The logic should verify the container is actually running, not just that the service is defined.

Suggested change
# Check if container is running (exact match) and www-data UID doesn't match host UID
CONTAINER_UID=$($WARDEN_BIN env exec "$container" id -u www-data 2>/dev/null || echo "")
if $WARDEN_BIN env ps --services 2>/dev/null | grep -q "^${container}$" \
&& [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then
# Ensure the container for this service is running before attempting to exec into it
CONTAINER_ID=$($WARDEN_BIN env ps -q "$container" 2>/dev/null || echo "")
if [[ -z "$CONTAINER_ID" ]]; then
continue
fi
CONTAINER_STATUS=$(docker container inspect "$CONTAINER_ID" --format '{{ .State.Status }}' 2>/dev/null || echo "")
if [[ "$CONTAINER_STATUS" != "running" ]]; then
continue
fi
# Check www-data UID inside the running container
CONTAINER_UID=$($WARDEN_BIN env exec "$container" id -u www-data 2>/dev/null || echo "")
if [[ -n "$CONTAINER_UID" ]] && [[ "$CONTAINER_UID" != "${HOST_UID}" ]]; then

Copilot uses AI. Check for mistakes.

# Update www-data user/group to match host UID/GID
$WARDEN_BIN env exec -u 0 "$container" usermod -u "${HOST_UID}" www-data 2>/dev/null || true

Copilot AI Jan 22, 2026

Copy link

Choose a reason for hiding this comment

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

The usermod command to change UID can fail if files owned by the www-data user are currently open or in use, or if the new UID conflicts with another existing user. While the '|| true' suppresses the error, silently failing could leave the system in an inconsistent state where some operations succeed and others fail. Consider checking the exit status and providing a warning message to the user if the operation fails.

Copilot uses AI. Check for mistakes.
$WARDEN_BIN env exec -u 0 "$container" groupmod -g "${HOST_GID}" www-data 2>/dev/null || true
Comment on lines +276 to +277

Copilot AI Jan 22, 2026

Copy link

Choose a reason for hiding this comment

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

The HOST_UID and HOST_GID variables are used without validation. If these values are empty or invalid (e.g., due to an environment issue), the usermod and groupmod commands will fail or produce unexpected results. Although these variables are set early in the script (lines 13-14), adding explicit validation before use would make the code more robust and provide clearer error messages if something goes wrong.

Copilot uses AI. Check for mistakes.

# Create backward-compatible group for original GID 1000 files (only if host GID differs)
if [[ "${HOST_GID}" != "1000" ]]; then
$WARDEN_BIN env exec -u 0 "$container" sh -c "getent group centos >/dev/null 2>&1 || groupadd -g 1000 centos" 2>/dev/null || true
$WARDEN_BIN env exec -u 0 "$container" usermod -aG centos www-data 2>/dev/null || true
Comment on lines +279 to +282

Copilot AI Jan 22, 2026

Copy link

Choose a reason for hiding this comment

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

The hardcoded group name "centos" may conflict with existing groups in the container or be confusing since it's not descriptive of its purpose. Consider using a more descriptive name like "legacy-files" or "original-gid" to better communicate that this group exists for backward compatibility with files created by the original GID 1000.

Suggested change
# Create backward-compatible group for original GID 1000 files (only if host GID differs)
if [[ "${HOST_GID}" != "1000" ]]; then
$WARDEN_BIN env exec -u 0 "$container" sh -c "getent group centos >/dev/null 2>&1 || groupadd -g 1000 centos" 2>/dev/null || true
$WARDEN_BIN env exec -u 0 "$container" usermod -aG centos www-data 2>/dev/null || true
# Create backward-compatible group "legacy-files" for original GID 1000 files (only if host GID differs)
if [[ "${HOST_GID}" != "1000" ]]; then
$WARDEN_BIN env exec -u 0 "$container" sh -c "getent group legacy-files >/dev/null 2>&1 || groupadd -g 1000 legacy-files" 2>/dev/null || true
$WARDEN_BIN env exec -u 0 "$container" usermod -aG legacy-files www-data 2>/dev/null || true

Copilot uses AI. Check for mistakes.
fi

# Restart container to apply changes
$WARDEN_BIN env restart "$container"

Copilot AI Jan 22, 2026

Copy link

Choose a reason for hiding this comment

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

Restarting containers sequentially in a loop could significantly slow down the startup process when multiple containers need permission fixes. Each restart waits for the previous container to fully restart before proceeding. Consider collecting all containers that need permission fixes and restarting them in parallel, or restart them all at once after all permission modifications are complete.

Copilot uses AI. Check for mistakes.
fi
done
fi
Comment on lines +266 to +289

Copilot AI Jan 22, 2026

Copy link

Choose a reason for hiding this comment

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

The permission fix block runs after Mutagen sync operations (lines 237-264) and can restart containers, which will invalidate the Mutagen sync connection that was just established. When a container is restarted on line 286, the container ID changes, breaking the Mutagen sync that checks for matching container IDs (line 243). This creates a race condition where Mutagen sync may be connected to a stale container.

The permission fix should run before Mutagen sync operations are initiated to avoid disrupting the sync connection.

Copilot uses AI. Check for mistakes.

## stop mutagen sync if needed
if [[ "${WARDEN_PARAMS[0]}" == "down" ]] \
&& [[ ${WARDEN_MUTAGEN_ENABLE} -eq 1 ]] && [[ -f "${MUTAGEN_SYNC_FILE}" ]]
Expand Down