permission fixes #42
permission fixes #42
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive permission fixes for database and filesystem access in a containerized application. The changes focus on robust user mapping configuration, directory permission management, and database initialization with extensive error handling and validation.
Key changes:
- Enhanced PUID/GUID user mapping with detailed logging and error handling
- Comprehensive directory and database permission validation with self-fixing capabilities
- Added database write permission checks in the Python application initialization
- Implemented post-startup database validation with background monitoring
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| docker-entrypoint.sh | Major overhaul of user mapping, directory setup, and database initialization with extensive validation and self-fixing logic |
| app/init.py | Added database write permission validation during application startup with error handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if ! sudo -u "#$target_uid" sqlite3 /app/instance/subscriptions.db "CREATE TABLE IF NOT EXISTS permission_test (id INTEGER); DROP TABLE IF EXISTS permission_test;" 2>/dev/null; then | ||
| echo "⚠️ Database write test failed - attempting repair" | ||
| # Try to fix any corruption or permission issues | ||
| chown "$target_uid:$target_gid" /app/instance/subscriptions.db* | ||
| chmod 664 /app/instance/subscriptions.db* | ||
| else | ||
| echo "✅ Database write test passed" |
There was a problem hiding this comment.
Using sudo with user ID instead of username can be risky if the UID doesn't correspond to a valid user. Consider checking if the user exists before using sudo, or handle the case where sudo fails gracefully.
| if ! sudo -u "#$target_uid" sqlite3 /app/instance/subscriptions.db "CREATE TABLE IF NOT EXISTS permission_test (id INTEGER); DROP TABLE IF EXISTS permission_test;" 2>/dev/null; then | |
| echo "⚠️ Database write test failed - attempting repair" | |
| # Try to fix any corruption or permission issues | |
| chown "$target_uid:$target_gid" /app/instance/subscriptions.db* | |
| chmod 664 /app/instance/subscriptions.db* | |
| else | |
| echo "✅ Database write test passed" | |
| if getent passwd "$target_uid" >/dev/null 2>&1; then | |
| if ! sudo -u "#$target_uid" sqlite3 /app/instance/subscriptions.db "CREATE TABLE IF NOT EXISTS permission_test (id INTEGER); DROP TABLE IF EXISTS permission_test;" 2>/dev/null; then | |
| echo "⚠️ Database write test failed - attempting repair" | |
| # Try to fix any corruption or permission issues | |
| chown "$target_uid:$target_gid" /app/instance/subscriptions.db* | |
| chmod 664 /app/instance/subscriptions.db* | |
| else | |
| echo "✅ Database write test passed" | |
| fi | |
| else | |
| echo "⚠️ No user exists with UID $target_uid; skipping database write test as that user." |
| if sudo -u "#$target_uid" python3 -c " | ||
| import sqlite3 | ||
| import sys | ||
| try: | ||
| conn = sqlite3.connect('$db_file') | ||
| conn.execute('CREATE TABLE IF NOT EXISTS permission_test (id INTEGER PRIMARY KEY)') | ||
| conn.execute('INSERT INTO permission_test DEFAULT VALUES') | ||
| conn.execute('DELETE FROM permission_test') | ||
| conn.execute('DROP TABLE permission_test') | ||
| conn.commit() | ||
| conn.close() | ||
| print('✅ Database write test PASSED') | ||
| except Exception as e: | ||
| print(f'❌ Database write test FAILED: {e}') | ||
| sys.exit(1) | ||
| " 2>/dev/null; then |
There was a problem hiding this comment.
The multi-line Python script uses shell variable interpolation ($db_file) which could be vulnerable to injection if the variable contains special characters. Consider using a safer approach like passing the database path as an environment variable or argument.
| sudo -u "#$target_uid" python3 -c " | ||
| import sqlite3 | ||
| import os | ||
| db_path = '$db_file' | ||
| if not os.path.exists(db_path): | ||
| conn = sqlite3.connect(db_path) | ||
| conn.execute('CREATE TABLE IF NOT EXISTS init_test (id INTEGER)') | ||
| conn.execute('DROP TABLE init_test') | ||
| conn.commit() | ||
| conn.close() | ||
| os.chmod(db_path, 0o664) | ||
| print('📝 Database pre-created with proper permissions') | ||
| " 2>/dev/null || echo "ℹ️ Database will be created by application" |
There was a problem hiding this comment.
Similar shell variable interpolation vulnerability exists here with $db_file. The variable should be properly escaped or passed through a safer mechanism.
| # Start validation in background | ||
| (sleep 5 && validate_database "$@") & |
There was a problem hiding this comment.
The hardcoded 5-second sleep may not be sufficient for all applications to start. Consider making this configurable via an environment variable or implementing a more robust wait mechanism that checks if the application is actually ready.
| conn.execute(text('CREATE TABLE IF NOT EXISTS permission_test (id INTEGER)')) | ||
| conn.execute(text('DROP TABLE IF EXISTS permission_test')) | ||
| conn.commit() |
There was a problem hiding this comment.
Creating and dropping a test table on every application startup adds unnecessary overhead. Consider using a simpler write test like inserting into an existing table or checking database file permissions instead.
No description provided.