read-only changes #43
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive read-only filesystem and user directive support for the Docker container, enabling enhanced security compliance while maintaining backward compatibility with existing PUID/GUID configurations.
- Enhanced container startup script with automatic detection of read-only filesystems and user directives
- Added comprehensive security documentation with deployment examples and troubleshooting guides
- Improved error handling and user feedback with detailed logging for different deployment scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| docker-entrypoint.sh | Completely rewritten user mapping logic with read-only filesystem detection, user directive support, and enhanced error handling |
| SECURITY_COMPATIBILITY.md | New comprehensive documentation covering security features, deployment modes, and migration guidance |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| echo "✅ User mapping configured: $APP_USER:$APP_GROUP" | ||
|
|
||
| elif [ "$(id -u)" = "0" ]; then | ||
| echo "� Root mode without PUID/GUID: Using build-time defaults" |
There was a problem hiding this comment.
Invalid character '�' should be replaced with appropriate emoji like '🔧'
| echo "� Root mode without PUID/GUID: Using build-time defaults" | |
| echo "⚠️ Root mode without PUID/GUID: Using build-time defaults" |
|
|
||
| # Fix ownership and permissions | ||
| if [ "$(id -u)" = "0" ]; then | ||
| if [ "$(id -u)" = "0" ] && ! is_user_directive; then |
There was a problem hiding this comment.
The logic here could fail if is_user_directive returns an error status code. The function should be called separately and its result checked to ensure proper error handling.
| if [ "$(id -u)" = "0" ] && ! is_user_directive; then | |
| is_user_directive | |
| is_user_directive_status=$? | |
| if [ "$(id -u)" = "0" ] && [ "$is_user_directive_status" -ne 0 ]; then |
| fi | ||
|
|
||
| # Additional check: if PUID/GUID are set but we can't modify users, likely user directive | ||
| if [ -n "$PUID" ] && [ -n "$GUID" ] && is_readonly_fs; then |
There was a problem hiding this comment.
Similar to the previous issue, if is_readonly_fs returns an error status code, this condition could behave unexpectedly. The function result should be captured and checked separately.
| if [ -n "$PUID" ] && [ -n "$GUID" ] && is_readonly_fs; then | |
| is_ro_fs=1 | |
| if is_readonly_fs; then | |
| is_ro_fs=0 | |
| fi | |
| if [ -n "$PUID" ] && [ -n "$GUID" ] && [ "$is_ro_fs" -eq 0 ]; then |
| touch /tmp/.write-test 2>/dev/null && rm -f /tmp/.write-test 2>/dev/null | ||
| return $? | ||
| # Check if root filesystem is read-only | ||
| if mount | grep -q 'on / .*ro,'; then |
There was a problem hiding this comment.
The regex pattern 'on / .ro,' may not match all read-only mount formats. Some systems might show 'ro' without a trailing comma or in different positions. Consider a more robust pattern like 'on / .\bro\b' to match 'ro' as a complete word.
| if mount | grep -q 'on / .*ro,'; then | |
| if mount | grep -E -q 'on / .*\bro\b'; then |
No description provided.