Skip to content

Conversation

@nfebe
Copy link
Contributor

@nfebe nfebe commented Jan 29, 2026

  • Add User and Subdirectories fields to TemplateMount for declaring container ownership requirements
  • Add ApplyMountOwnership to set correct UID:GID on bind mount dirs
  • Inspect container user after start and apply ownership automatically
  • Convert template volumes from named to bind mounts (wordpress, ghost, nextcloud)
  • Add user field to template metadata (www-data 33:33, bitnami 1000:1000)
  • Add Laravel storage subdirectories (framework/cache, sessions, views)

Closes #50

@sourceant
Copy link

sourceant bot commented Jan 29, 2026

Code Review Summary

This pull request introduces a crucial feature: automated and intelligent handling of bind mount permissions for Docker Compose deployments, especially when using templates. This significantly enhances security, robustness, and compatibility with non-root containers. The changes involve modifying template metadata to include ownership information, updating the Docker discovery and manager to parse and apply these permissions, and adjusting existing templates to leverage this new capability.

🚀 Key Improvements

  • Enhanced Security: Bind mounts created via templates now specify explicit UID:GID ownership, moving away from the insecure default of 0777.
  • Improved Compatibility: Non-root containers, which often require specific user permissions for their data volumes, will now function correctly out-of-the-box with template deployments.
  • Robust Ownership Application: Ownership is applied dynamically post-container start by inspecting the running container's user, ensuring the correct permissions are set.
  • Cleaner Compose Files: Named volumes have been replaced with bind mounts in templates, simplifying the docker-compose.yml structure by removing redundant volume definitions.

💡 Minor Suggestions

  • Ensure the TemplateMount struct in internal/api/server.go is fully synchronized with the one in internal/docker/discovery.go to prevent potential API response inconsistencies if this struct is directly exposed.
  • Review the default permissions for createBindMountDirs for non-template scenarios. While ApplyMountOwnership overrides it, a default of 0755 might be more secure than 0777 if no specific ownership is provided.

🚨 Critical Issues

  • The name fields in multiple template metadata.yml files (ghost, laravel, nextcloud) are incorrectly set to WordPress. These should be updated to reflect the actual template name.

Copy link

@sourceant sourceant bot left a comment

Choose a reason for hiding this comment

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

Review complete. See the overview comment for a summary.

@flatrun flatrun deleted a comment from openhands-ai bot Jan 29, 2026
- Add User and Subdirectories fields to TemplateMount for declaring
  container ownership requirements
- Add ApplyMountOwnership to set correct UID:GID on bind mount dirs
- Inspect container user after start and apply ownership automatically
- Convert template volumes from named to bind mounts (wordpress, ghost,
  nextcloud)
- Add user field to template metadata (www-data 33:33, bitnami 1000:1000)
- Add Laravel storage subdirectories (framework/cache, sessions, views)

Closes #50

Signed-off-by: nfebe <fenn25.fn@gmail.com>
@nfebe nfebe force-pushed the fix/50-bind-mount-permissions branch from 4cff2cb to 1c76c91 Compare January 29, 2026 18:39
@sourceant
Copy link

sourceant bot commented Jan 29, 2026

🔍 Code Review

💡 1. **internal/docker/discovery.go** (Lines 281-289) - SECURITY

The createBindMountDirs function still sets 0777 permissions unconditionally. While ApplyMountOwnership will later apply more specific permissions for template deployments, for custom or non-template deployments, 0777 might be overly permissive and a potential security risk. Consider defaulting to 0755 (rwx for owner, rx for group/others) and only using 0777 if explicitly required by a configuration flag, or relying entirely on ApplyMountOwnership for all permission setting.

Suggested Code:

if err := os.MkdirAll(fullPath, 0755); err != nil {
	return err
}
// The permissions for bind mounts will be further refined by ApplyMountOwnership
// If not managed by a template, default permissions are 0755 to be less permissive.
if err := os.Chmod(fullPath, 0755); err != nil {
	return err
}

Current Code:

if err := os.MkdirAll(fullPath, 0777); err != nil {
	return err
}
if err := os.Chmod(fullPath, 0777); err != nil {
	return err
}
💡 2. **internal/api/server.go** (Lines 1795-1807) - REFACTOR

The TemplateMount struct definition was updated in internal/docker/discovery.go to include User and Subdirectories. The definition in internal/api/server.go should also be updated to match the latest fields for consistency and to avoid potential issues if this struct is marshaled/unmarshaled in other contexts within the API package or if the API package eventually generates OpenAPI specs from this struct.

Suggested Code:

type TemplateMount struct {
	ID             string   `json:"id" yaml:"id"`
	Name           string   `json:"name" yaml:"name"`
	ContainerPath  string   `json:"container_path" yaml:"container_path"`
	Description    string   `json:"description" yaml:"description"`
	Type           string   `json:"type" yaml:"type"`
	Required       bool     `json:"required" yaml:"required"`
	User           string   `json:"user,omitempty" yaml:"user,omitempty"`
	Subdirectories []string `json:"subdirectories,omitempty" yaml:"subdirectories,omitempty"`
}

Current Code:

type TemplateMount struct {
	ID            string `json:"id" yaml:"id"`
	Name          string `json:"name" yaml:"name"`
	ContainerPath string `json:"container_path" yaml:"container_path"`
	Description   string `json:"description" yaml:"description"`
	Type          string `json:"type" yaml:"type"`
	Required      bool   `json:"required" yaml:"required"`
}
💡 3. **templates/ghost/metadata.yml** (Line 1) - BUG

The name field in this metadata.yml is WordPress which is incorrect for the ghost template. This should be updated to Ghost.

Suggested Code:

name: Ghost

Current Code:

name: WordPress
💡 4. **templates/nextcloud/metadata.yml** (Line 1) - BUG

The name field in this metadata.yml is WordPress which is incorrect for the nextcloud template. This should be updated to Nextcloud.

Suggested Code:

name: Nextcloud

Current Code:

name: WordPress

Verdict: APPROVE

Posted as a comment because posting a review failed.

@nfebe nfebe merged commit bb9b385 into main Jan 29, 2026
5 checks passed
@nfebe nfebe deleted the fix/50-bind-mount-permissions branch January 29, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Container permission issues with bind-mounted volumes

2 participants