add security fixes, port hardening, and expanded QA coverage
Security fixes: - Replace debug=True with env-driven FLASK_DEBUG in app.py - Add _safe_path helper and path-traversal protection to all 6 file routes in file_manager.py - Add peer_name regex and input validation (public_key, name, endpoint_ip) in wireguard_manager.py - Stop returning private key from GET /api/wireguard/keys; return only public_key + has_private_key boolean - Fix is_local_request() XFF bypass by checking remote_addr only, ignoring X-Forwarded-For - Remove duplicate get_all_configs / get_config_summary methods from config_manager.py DevOps: - Bind 6 internal service ports to 127.0.0.1 in docker-compose.yml (radicale, webdav, api, webui, rainloop, filegator) - Move WebDAV credentials to env vars (WEBDAV_USER, WEBDAV_PASS) - Pin flask, flask-cors, requests, cryptography, docker to secure minimum versions in requirements.txt QA (560 tests, 0 failures): - tests/test_wireguard_endpoints.py: 18 new endpoint tests - tests/test_file_endpoints.py: 24 new endpoint tests incl. path traversal - tests/test_container_manager.py: expanded from 2 to 30 tests - tests/test_config_backup_restore_http.py: 25 new tests (new file) - tests/test_config_apply.py: 9 new tests (new file) Docs: - Rewrite README.md with accurate architecture, ports, env vars, security notes - Rewrite QUICKSTART.md with verified commands Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
+30
-13
@@ -343,8 +343,16 @@ def _local_subnets():
|
||||
|
||||
|
||||
def is_local_request():
|
||||
# SECURITY: do NOT use X-Forwarded-For for auth. Caddy (and any reverse
|
||||
# proxy) sets XFF to the original client IP, but the TCP peer that reaches
|
||||
# this Flask process is always the proxy itself (an RFC-1918 Docker IP).
|
||||
# Trusting XFF would let any internet client claim a local IP via that
|
||||
# header. Only the direct TCP peer (request.remote_addr) is trustworthy:
|
||||
# all legitimate local traffic comes directly from the Docker network or
|
||||
# loopback, so remote_addr being local is a sufficient and necessary
|
||||
# condition. The XFF header is read for logging only, never for access
|
||||
# decisions.
|
||||
remote_addr = request.remote_addr
|
||||
forwarded_for = request.headers.get('X-Forwarded-For', '')
|
||||
|
||||
def _allowed(addr):
|
||||
if not addr:
|
||||
@@ -374,14 +382,7 @@ def is_local_request():
|
||||
pass
|
||||
return False
|
||||
|
||||
if _allowed(remote_addr):
|
||||
return True
|
||||
# Only trust the LAST X-Forwarded-For entry — that is what the reverse proxy appended.
|
||||
if forwarded_for:
|
||||
last_hop = forwarded_for.split(',')[-1].strip()
|
||||
if _allowed(last_hop):
|
||||
return True
|
||||
return False
|
||||
return _allowed(remote_addr)
|
||||
|
||||
@app.route('/health', methods=['GET'])
|
||||
def health_check():
|
||||
@@ -1416,10 +1417,13 @@ def test_network():
|
||||
# WireGuard API
|
||||
@app.route('/api/wireguard/keys', methods=['GET'])
|
||||
def get_wireguard_keys():
|
||||
"""Get WireGuard keys."""
|
||||
"""Get WireGuard keys (public key only; private key never leaves the server)."""
|
||||
try:
|
||||
result = wireguard_manager.get_keys()
|
||||
return jsonify(result)
|
||||
keys = wireguard_manager.get_keys()
|
||||
return jsonify({
|
||||
'public_key': keys.get('public_key', ''),
|
||||
'has_private_key': bool(keys.get('private_key')),
|
||||
})
|
||||
except Exception as e:
|
||||
logger.error(f"Error getting WireGuard keys: {e}")
|
||||
return jsonify({"error": str(e)}), 500
|
||||
@@ -2149,6 +2153,8 @@ def create_folder():
|
||||
return jsonify({"error": "No data provided"}), 400
|
||||
result = file_manager.create_folder(data)
|
||||
return jsonify(result)
|
||||
except ValueError as e:
|
||||
return jsonify({"error": str(e)}), 400
|
||||
except Exception as e:
|
||||
logger.error(f"Error creating folder: {e}")
|
||||
return jsonify({"error": str(e)}), 500
|
||||
@@ -2159,6 +2165,8 @@ def delete_folder(username, folder_path):
|
||||
try:
|
||||
result = file_manager.delete_folder(username, folder_path)
|
||||
return jsonify(result)
|
||||
except ValueError as e:
|
||||
return jsonify({"error": str(e)}), 400
|
||||
except Exception as e:
|
||||
logger.error(f"Error deleting folder: {e}")
|
||||
return jsonify({"error": str(e)}), 500
|
||||
@@ -2175,6 +2183,8 @@ def upload_file(username):
|
||||
|
||||
result = file_manager.upload_file(username, file, path)
|
||||
return jsonify(result)
|
||||
except ValueError as e:
|
||||
return jsonify({"error": str(e)}), 400
|
||||
except Exception as e:
|
||||
logger.error(f"Error uploading file: {e}")
|
||||
return jsonify({"error": str(e)}), 500
|
||||
@@ -2185,6 +2195,8 @@ def download_file(username, file_path):
|
||||
try:
|
||||
result = file_manager.download_file(username, file_path)
|
||||
return jsonify(result)
|
||||
except ValueError as e:
|
||||
return jsonify({"error": str(e)}), 400
|
||||
except Exception as e:
|
||||
logger.error(f"Error downloading file: {e}")
|
||||
return jsonify({"error": str(e)}), 500
|
||||
@@ -2195,6 +2207,8 @@ def delete_file(username, file_path):
|
||||
try:
|
||||
result = file_manager.delete_file(username, file_path)
|
||||
return jsonify(result)
|
||||
except ValueError as e:
|
||||
return jsonify({"error": str(e)}), 400
|
||||
except Exception as e:
|
||||
logger.error(f"Error deleting file: {e}")
|
||||
return jsonify({"error": str(e)}), 500
|
||||
@@ -2206,6 +2220,8 @@ def list_files(username):
|
||||
folder = request.args.get('folder', '')
|
||||
result = file_manager.list_files(username, folder)
|
||||
return jsonify(result)
|
||||
except ValueError as e:
|
||||
return jsonify({"error": str(e)}), 400
|
||||
except Exception as e:
|
||||
logger.error(f"Error listing files: {e}")
|
||||
return jsonify({"error": str(e)}), 500
|
||||
@@ -2915,4 +2931,5 @@ def remove_volume(name):
|
||||
return jsonify({'removed': success})
|
||||
|
||||
if __name__ == '__main__':
|
||||
app.run(host='0.0.0.0', port=3000, debug=True)
|
||||
debug = os.environ.get('FLASK_DEBUG', '0') == '1'
|
||||
app.run(host='0.0.0.0', port=3000, debug=debug)
|
||||
@@ -196,21 +196,6 @@ class ConfigManager:
|
||||
"warnings": warnings
|
||||
}
|
||||
|
||||
def get_all_configs(self) -> Dict[str, Dict]:
|
||||
"""Return all stored service configurations."""
|
||||
return dict(self.configs)
|
||||
|
||||
def get_config_summary(self) -> Dict[str, Any]:
|
||||
"""Return a high-level summary of configuration state."""
|
||||
backup_count = sum(
|
||||
1 for p in self.backup_dir.iterdir() if p.is_dir()
|
||||
) if self.backup_dir.exists() else 0
|
||||
return {
|
||||
'total_services': len(self.service_schemas),
|
||||
'configured_services': len(self.configs),
|
||||
'backup_count': backup_count,
|
||||
}
|
||||
|
||||
def backup_config(self) -> str:
|
||||
"""Create a backup of cell_config.json, secrets, Caddyfile, .env, Corefile, and DNS zones."""
|
||||
try:
|
||||
|
||||
+29
-6
@@ -5,6 +5,7 @@ Handles WebDAV file storage services
|
||||
"""
|
||||
|
||||
import os
|
||||
import re
|
||||
import json
|
||||
import subprocess
|
||||
import logging
|
||||
@@ -43,6 +44,28 @@ class FileManager(BaseServiceManager):
|
||||
except (PermissionError, OSError):
|
||||
pass
|
||||
|
||||
def _safe_path(self, username: str, *parts: str) -> str:
|
||||
"""Resolve a safe path under files_dir/username.
|
||||
|
||||
Whitelists username, joins extra parts, resolves to a real path, and
|
||||
asserts the result is contained within the user's directory. Raises
|
||||
ValueError on any sign of path traversal or invalid input.
|
||||
"""
|
||||
if not isinstance(username, str) or not re.match(r'^[A-Za-z0-9_.-]{1,64}$', username):
|
||||
raise ValueError(f"Invalid username: {username!r}")
|
||||
safe_parts = []
|
||||
for p in parts:
|
||||
if p is None:
|
||||
continue
|
||||
if not isinstance(p, str):
|
||||
raise ValueError(f"Invalid path component: {p!r}")
|
||||
safe_parts.append(p)
|
||||
user_root = os.path.realpath(os.path.join(self.files_dir, username))
|
||||
candidate = os.path.realpath(os.path.join(self.files_dir, username, *safe_parts))
|
||||
if candidate != user_root and not candidate.startswith(user_root + os.sep):
|
||||
raise ValueError(f"Path traversal detected for user {username!r}: {parts!r}")
|
||||
return candidate
|
||||
|
||||
def _generate_webdav_config(self):
|
||||
"""Generate WebDAV configuration"""
|
||||
config = """# WebDAV configuration for Personal Internet Cell
|
||||
@@ -230,7 +253,7 @@ umask = 022
|
||||
logger.error("Username and folder_path must not be empty")
|
||||
return False
|
||||
try:
|
||||
full_path = os.path.join(self.files_dir, username, folder_path)
|
||||
full_path = self._safe_path(username, folder_path)
|
||||
os.makedirs(full_path, exist_ok=True)
|
||||
|
||||
logger.info(f"Created folder {folder_path} for {username}")
|
||||
@@ -246,7 +269,7 @@ umask = 022
|
||||
logger.error("Username and folder_path must not be empty")
|
||||
return False
|
||||
try:
|
||||
full_path = os.path.join(self.files_dir, username, folder_path)
|
||||
full_path = self._safe_path(username, folder_path)
|
||||
|
||||
if os.path.exists(full_path):
|
||||
shutil.rmtree(full_path)
|
||||
@@ -263,7 +286,7 @@ umask = 022
|
||||
def upload_file(self, username: str, file_path: str, file_data: bytes) -> bool:
|
||||
"""Upload a file for a user"""
|
||||
try:
|
||||
full_path = os.path.join(self.files_dir, username, file_path)
|
||||
full_path = self._safe_path(username, file_path)
|
||||
|
||||
# Ensure directory exists
|
||||
os.makedirs(os.path.dirname(full_path), exist_ok=True)
|
||||
@@ -282,7 +305,7 @@ umask = 022
|
||||
def download_file(self, username: str, file_path: str) -> Optional[bytes]:
|
||||
"""Download a file for a user"""
|
||||
try:
|
||||
full_path = os.path.join(self.files_dir, username, file_path)
|
||||
full_path = self._safe_path(username, file_path)
|
||||
|
||||
if os.path.exists(full_path):
|
||||
with open(full_path, 'rb') as f:
|
||||
@@ -298,7 +321,7 @@ umask = 022
|
||||
def delete_file(self, username: str, file_path: str) -> bool:
|
||||
"""Delete a file for a user"""
|
||||
try:
|
||||
full_path = os.path.join(self.files_dir, username, file_path)
|
||||
full_path = self._safe_path(username, file_path)
|
||||
|
||||
if os.path.exists(full_path):
|
||||
os.remove(full_path)
|
||||
@@ -317,7 +340,7 @@ umask = 022
|
||||
files = []
|
||||
|
||||
try:
|
||||
full_path = os.path.join(self.files_dir, username, folder_path)
|
||||
full_path = self._safe_path(username, folder_path)
|
||||
|
||||
if os.path.exists(full_path):
|
||||
for item in os.listdir(full_path):
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
flask==2.3.3
|
||||
flask-cors==4.0.0
|
||||
requests==2.31.0
|
||||
cryptography==41.0.7
|
||||
flask>=3.0.3
|
||||
flask-cors>=4.0.1
|
||||
requests>=2.32.3
|
||||
cryptography>=42.0.5
|
||||
pyyaml==6.0.1
|
||||
icalendar==5.0.7
|
||||
vobject==0.9.6.1
|
||||
@@ -13,4 +13,4 @@ pytest==7.4.3
|
||||
pytest-cov==4.1.0
|
||||
pytest-mock==3.12.0
|
||||
|
||||
docker
|
||||
docker>=7.0.0
|
||||
@@ -4,6 +4,7 @@ WireGuard Manager for Personal Internet Cell
|
||||
"""
|
||||
|
||||
import os
|
||||
import re
|
||||
import json
|
||||
import base64
|
||||
import socket
|
||||
@@ -92,6 +93,8 @@ class WireGuardManager(BaseServiceManager):
|
||||
|
||||
def generate_peer_keys(self, peer_name: str) -> Dict[str, str]:
|
||||
"""Generate a keypair for a peer, save to keys_dir/peers/, return as base64."""
|
||||
if not isinstance(peer_name, str) or not re.match(r'^[A-Za-z0-9_.-]{1,64}$', peer_name):
|
||||
raise ValueError(f"Invalid peer_name: {peer_name!r}")
|
||||
priv_bytes, pub_bytes = self._generate_keypair()
|
||||
priv_b64 = base64.b64encode(priv_bytes).decode()
|
||||
pub_b64 = base64.b64encode(pub_bytes).decode()
|
||||
@@ -332,7 +335,16 @@ class WireGuardManager(BaseServiceManager):
|
||||
Passing full-tunnel or split-tunnel CIDRs here would cause the server
|
||||
to route all internet or LAN traffic to that peer — breaking everything.
|
||||
"""
|
||||
import ipaddress
|
||||
import ipaddress, re as _re
|
||||
if not isinstance(public_key, str) or not _re.match(r'^[A-Za-z0-9+/]{43}=$', public_key.strip()):
|
||||
return False # invalid WireGuard public key
|
||||
if name and not _re.match(r'^[A-Za-z0-9_. -]{1,64}$', name):
|
||||
return False # reject names with newlines/brackets
|
||||
if endpoint_ip:
|
||||
try:
|
||||
ipaddress.ip_address(endpoint_ip.strip())
|
||||
except ValueError:
|
||||
return False
|
||||
try:
|
||||
# Enforce /32: reject any CIDR wider than a single host
|
||||
for cidr in (c.strip() for c in allowed_ips.split(',')):
|
||||
|
||||
Reference in New Issue
Block a user