fix(security): enforce files.read on reads; de-shell syslog unit filter

Follow-ups from the GHSA-4wqh-7f4f-5qmx / GHSA-mc93-rc3x-fpgq review:

- File Manager read endpoints (browse/info/read/search/disk-*/analyze/
  type-breakdown/download + s3/browse, s3/read, s3/download-url) only
  required a valid JWT, so a user whose files.read permission is revoked
  via custom permissions could still read and download files. They now
  require permission_required('files', 'read'), matching the write side.

- LogService._read_syslog() interpolated the unit name into a bash -c
  string using subprocess.list2cmdline(), which is cmd.exe-style quoting
  and does not stop $(...)/backtick expansion under bash. grep now runs
  as a plain argv list (with --) and the tail happens in Python.
  Reachable only on non-systemd hosts by admins, but the same bug class
  as the Postfix advisory.

Regression tests: revoked files.read 403s every read endpoint; the
syslog filter never invokes a shell and passes payloads verbatim.

backend/app/api/files.py

@@ -1,7 +1,6 @@
 """File Manager API endpoints for browsing, editing, and managing files."""
 
 from flask import Blueprint, request, jsonify, send_file
-from flask_jwt_extended import jwt_required
 from ..middleware.rbac import permission_required
 from ..services.file_service import FileService
 from ..services.storage_provider_service import StorageProviderService
@@ -12,7 +11,7 @@ files_bp = Blueprint('files', __name__)
 
 
 @files_bp.route('/browse', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def browse_directory():
     """List directory contents."""
     path = request.args.get('path', '/home')
@@ -26,7 +25,7 @@ def browse_directory():
 
 
 @files_bp.route('/info', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def get_file_info():
     """Get information about a file or directory."""
     path = request.args.get('path')
@@ -47,7 +46,7 @@ def get_file_info():
 
 
 @files_bp.route('/read', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def read_file():
     """Read file contents."""
     path = request.args.get('path')
@@ -254,7 +253,7 @@ def change_permissions():
 
 
 @files_bp.route('/search', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def search_files():
     """Search for files matching a pattern."""
     directory = request.args.get('directory', '/home')
@@ -274,7 +273,7 @@ def search_files():
 
 
 @files_bp.route('/disk-usage', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def get_disk_usage():
     """Get disk usage for a path."""
     path = request.args.get('path', '/')
@@ -287,7 +286,7 @@ def get_disk_usage():
 
 
 @files_bp.route('/disk-mounts', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def get_disk_mounts():
     """Get disk usage for all mount points."""
     result = FileService.get_all_disk_mounts()
@@ -298,7 +297,7 @@ def get_disk_mounts():
 
 
 @files_bp.route('/analyze', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def analyze_directory():
     """Analyze directory sizes."""
     path = request.args.get('path', '/home')
@@ -315,7 +314,7 @@ def analyze_directory():
 
 
 @files_bp.route('/type-breakdown', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def get_type_breakdown():
     """Get file type breakdown for a directory."""
     path = request.args.get('path', '/home')
@@ -331,7 +330,7 @@ def get_type_breakdown():
 
 
 @files_bp.route('/download', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def download_file():
     """Download a file."""
     path = request.args.get('path')
@@ -420,7 +419,7 @@ def upload_file():
 # ── S3 / object-storage browser (reuses the configured backup storage creds) ──
 
 @files_bp.route('/s3/browse', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def s3_browse():
     """List a bucket prefix in the same entry shape as the local browser."""
     path = request.args.get('path', '/')
@@ -429,7 +428,7 @@ def s3_browse():
 
 
 @files_bp.route('/s3/read', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def s3_read():
     """Read a text object for in-app editing."""
     path = request.args.get('path')
@@ -466,7 +465,7 @@ def s3_delete():
 
 
 @files_bp.route('/s3/download-url', methods=['GET'])
-@jwt_required()
+@permission_required('files', 'read')
 def s3_download_url():
     """Return a short-lived presigned URL the browser can download directly."""
     path = request.args.get('path')

backend/app/services/log_service.py

@@ -315,8 +315,11 @@ class LogService:
         """Read system logs from a syslog file, optionally filtering by service."""
         try:
             if service:
+                # No shell: list2cmdline() is cmd.exe-style quoting and does
+                # not protect against $(...)/backtick expansion under bash.
+                # grep runs as argv and the tail happens in Python instead.
                 result = run_privileged(
-                    ['bash', '-c', f'grep -i {subprocess.list2cmdline([service])} {subprocess.list2cmdline([filepath])} | tail -n {int(lines)}'],
+                    ['grep', '-i', '--', service, filepath],
                     timeout=60,
                 )
             else:
@@ -327,6 +330,10 @@ class LogService:
 
             if result.returncode == 0 or (service and result.returncode == 1):
                 log_lines = result.stdout.split('\n') if result.stdout else []
+                if log_lines and log_lines[-1] == '':
+                    log_lines.pop()  # trailing newline artifact
+                if service:
+                    log_lines = log_lines[-int(lines):]
                 return sourced_result(log_lines, 'syslog', filepath)
             else:
                 return {'success': False, 'error': result.stderr}

backend/tests/test_files_rbac.py

@@ -8,6 +8,8 @@ GHSA-mc93-rc3x-fpgq — PostfixService.install() interpolated the hostname into
 a bash -c string. Hostnames are now validated and debconf values are piped
 via stdin, never through a shell.
 """
+import subprocess
+
 import pytest
 
 from app import db as _db
@@ -90,6 +92,89 @@ def test_viewer_can_still_browse(client, role_headers):
     assert resp.status_code != 403
 
 
+# --------------------------------------------------------------------------- #
+# files.read revocation: a user whose files.read permission is disabled must
+# be blocked from read endpoints too, not just writes
+# --------------------------------------------------------------------------- #
+
+READ_CASES = [
+    '/api/v1/files/browse?path=/tmp',
+    '/api/v1/files/info?path=/tmp/x',
+    '/api/v1/files/read?path=/tmp/x',
+    '/api/v1/files/search?path=/tmp&query=x',
+    '/api/v1/files/download?path=/tmp/x',
+    '/api/v1/files/s3/browse?path=/',
+    '/api/v1/files/s3/read?path=/x',
+    '/api/v1/files/s3/download-url?path=/x',
+]
+
+
+@pytest.fixture
+def no_read_headers(app):
+    """JWT headers for a viewer whose files.read permission is revoked."""
+    from flask_jwt_extended import create_access_token
+    from werkzeug.security import generate_password_hash
+
+    with app.app_context():
+        u = User(email='files_noread@t.local', username='files_noread',
+                 password_hash=generate_password_hash('x'),
+                 role=User.ROLE_VIEWER, is_active=True)
+        perms = {f: {'read': True, 'write': False} for f in User.PERMISSION_FEATURES}
+        perms['files'] = {'read': False, 'write': False}
+        u.set_permissions(perms)
+        _db.session.add(u)
+        _db.session.commit()
+        return {'Authorization': f'Bearer {create_access_token(identity=u.id)}'}
+
+
+@pytest.mark.parametrize('url', READ_CASES)
+def test_revoked_files_read_blocks_read_endpoints(client, no_read_headers, url):
+    resp = client.get(url, headers=no_read_headers)
+    assert resp.status_code == 403
+    assert 'files' in resp.get_json()['error']
+
+
+# --------------------------------------------------------------------------- #
+# log_service._read_syslog: the service filter must never go through a shell
+# (subprocess.list2cmdline is cmd.exe quoting and does not stop $(...)/backtick
+# expansion under bash)
+# --------------------------------------------------------------------------- #
+
+def test_read_syslog_does_not_use_a_shell(monkeypatch):
+    from app.services import log_service
+    from app.services.log_service import LogService
+
+    captured = {}
+
+    def fake_run_privileged(cmd, **kwargs):
+        captured['cmd'] = cmd
+        return subprocess.CompletedProcess(cmd, 0, stdout='match1\nmatch2\n', stderr='')
+
+    monkeypatch.setattr(log_service, 'run_privileged', fake_run_privileged)
+
+    payload = 'x$(id)`id`'
+    result = LogService._read_syslog('/var/log/syslog', payload, 100)
+
+    cmd = captured['cmd']
+    assert 'bash' not in cmd and '-c' not in cmd
+    # the payload travels as a standalone argv element, verbatim
+    assert payload in cmd
+    assert result['success']
+
+
+def test_read_syslog_tails_matches_in_python(monkeypatch):
+    from app.services import log_service
+    from app.services.log_service import LogService
+
+    def fake_run_privileged(cmd, **kwargs):
+        return subprocess.CompletedProcess(cmd, 0, stdout='l1\nl2\nl3\n', stderr='')
+
+    monkeypatch.setattr(log_service, 'run_privileged', fake_run_privileged)
+
+    result = LogService._read_syslog('/var/log/syslog', 'nginx', 2)
+    assert result['lines'] == ['l2', 'l3']
+
+
 # --------------------------------------------------------------------------- #
 # GHSA-mc93-rc3x-fpgq: postfix install rejects malicious hostnames
 # --------------------------------------------------------------------------- #