From dd9003d4580efaa3dbc0f9eb437e47841e373863 Mon Sep 17 00:00:00 2001 From: Juan Denis Date: Tue, 3 Nov 2026 12:54:13 -0500 Subject: [PATCH] fix(security): replace bare excepts, stop leaking exception text Bare excepts in sockets/servers/system now catch specific errors and log; malformed metrics from/to returns 400. docker cleanup and update-check no longer echo str(e). --- backend/app/api/docker.py | 39 ++++++++++++++-------- backend/app/api/servers.py | 8 ++--- backend/app/api/servers_agents.py | 3 +- backend/app/api/system.py | 5 +-- backend/app/sockets.py | 16 +++++---- backend/tests/test_servers_list_metrics.py | 14 ++++++++ 6 files changed, 58 insertions(+), 27 deletions(-) diff --git a/backend/app/api/docker.py b/backend/app/api/docker.py index 86fcc648..7aeec0b4 100644 --- a/backend/app/api/docker.py +++ b/backend/app/api/docker.py @@ -1,3 +1,5 @@ +import logging + from flask import Blueprint, request, jsonify from flask_jwt_extended import jwt_required, get_jwt_identity from app.models import User, Application @@ -7,6 +9,8 @@ from app import db, paths from app.error_reporting import unexpected_response from app.api._request import json_object +logger = logging.getLogger(__name__) + docker_bp = Blueprint('docker', __name__) @@ -505,8 +509,9 @@ def docker_cleanup(): } else: results['containers'] = {'removed': 0, 'success': True} - except Exception as e: - results['containers'] = {'error': str(e)} + except Exception: + logger.exception('Docker cleanup step failed') + results['containers'] = {'error': 'Cleanup step failed; see server log'} # Remove dangling images try: @@ -515,8 +520,9 @@ def docker_cleanup(): 'success': result['success'], 'output': result['output'].strip() } - except Exception as e: - results['images'] = {'error': str(e)} + except Exception: + logger.exception('Docker cleanup step failed') + results['images'] = {'error': 'Cleanup step failed; see server log'} # Remove unused networks try: @@ -525,8 +531,9 @@ def docker_cleanup(): 'success': result['success'], 'output': result['output'].strip() } - except Exception as e: - results['networks'] = {'error': str(e)} + except Exception: + logger.exception('Docker cleanup step failed') + results['networks'] = {'error': 'Cleanup step failed; see server log'} # Remove unused volumes (optional) if include_volumes: @@ -536,8 +543,9 @@ def docker_cleanup(): 'success': result['success'], 'output': result['output'].strip() } - except Exception as e: - results['volumes'] = {'error': str(e)} + except Exception: + logger.exception('Docker cleanup step failed') + results['volumes'] = {'error': 'Cleanup step failed; see server log'} else: results['volumes'] = {'skipped': True, 'message': 'Volume cleanup not requested'} @@ -581,24 +589,27 @@ def cleanup_all_apps(): try: compose_result = DockerService.compose_down(app.root_path, volumes=True, remove_orphans=True) app_result['steps'].append({'action': 'docker_down', 'success': compose_result.get('success', False)}) - except Exception as e: - app_result['steps'].append({'action': 'docker_down', 'error': str(e)}) + except Exception: + logger.exception('App cleanup step failed') + app_result['steps'].append({'action': 'docker_down', 'error': 'Step failed; see server log'}) app_result['success'] = False # Delete folder try: shutil.rmtree(app.root_path) app_result['steps'].append({'action': 'delete_folder', 'success': True}) - except Exception as e: - app_result['steps'].append({'action': 'delete_folder', 'error': str(e)}) + except Exception: + logger.exception('App cleanup step failed') + app_result['steps'].append({'action': 'delete_folder', 'error': 'Step failed; see server log'}) app_result['success'] = False # Remove from database try: db.session.delete(app) app_result['steps'].append({'action': 'delete_db', 'success': True}) - except Exception as e: - app_result['steps'].append({'action': 'delete_db', 'error': str(e)}) + except Exception: + logger.exception('App cleanup step failed') + app_result['steps'].append({'action': 'delete_db', 'error': 'Step failed; see server log'}) app_result['success'] = False results.append(app_result) diff --git a/backend/app/api/servers.py b/backend/app/api/servers.py index b1c81b8e..44121353 100644 --- a/backend/app/api/servers.py +++ b/backend/app/api/servers.py @@ -883,15 +883,15 @@ def get_server_metrics(server_id): try: from_dt = datetime.fromisoformat(from_time.replace('Z', '+00:00')) query = query.filter(ServerMetrics.timestamp >= from_dt) - except: - pass + except ValueError: + raise ValidationError("Invalid 'from' timestamp; use ISO 8601") if to_time: try: to_dt = datetime.fromisoformat(to_time.replace('Z', '+00:00')) query = query.filter(ServerMetrics.timestamp <= to_dt) - except: - pass + except ValueError: + raise ValidationError("Invalid 'to' timestamp; use ISO 8601") metrics = query.order_by(ServerMetrics.timestamp.desc()).limit(limit).all() diff --git a/backend/app/api/servers_agents.py b/backend/app/api/servers_agents.py index db47e704..34ed92cf 100644 --- a/backend/app/api/servers_agents.py +++ b/backend/app/api/servers_agents.py @@ -409,7 +409,8 @@ def _compare_versions(v1, v2): elif a > b: return 1 return 0 - except: + except (ValueError, AttributeError, TypeError): + current_app.logger.warning('Could not compare versions %r and %r', v1, v2) return 0 diff --git a/backend/app/api/system.py b/backend/app/api/system.py index 68b5b9f0..39aa3849 100644 --- a/backend/app/api/system.py +++ b/backend/app/api/system.py @@ -84,7 +84,7 @@ def check_update(): def parse_version(v): try: return tuple(int(x) for x in v.split('.')) - except: + except ValueError: return (0, 0, 0) if parse_version(latest_version) > parse_version(current_version): @@ -96,7 +96,8 @@ def check_update(): else: result['error'] = f'GitHub API error: {e.code}' except Exception as e: - result['error'] = f'Failed to check for updates: {str(e)}' + current_app.logger.warning('Update check failed: %s', e) + result['error'] = 'Failed to check for updates' # Cache the result _update_cache['last_check'] = time.time() diff --git a/backend/app/sockets.py b/backend/app/sockets.py index a20153ce..6bb0c1a5 100644 --- a/backend/app/sockets.py +++ b/backend/app/sockets.py @@ -5,6 +5,8 @@ import threading import time import queue import re +import logging +import subprocess from app.services.system_service import SystemService from app.services.log_service import LogService, LogStreamer @@ -12,6 +14,8 @@ from app.services.docker_service import DockerService from app import sockets_rooms as rooms from app.utils.background_loop import BackgroundLoop +logger = logging.getLogger(__name__) + class AuthorizedSocketIO(SocketIO): """Recheck audience authorization before every server-side delivery. @@ -775,11 +779,11 @@ def handle_subscribe_container_logs(data): try: process.terminate() process.wait(timeout=2) - except: + except (OSError, subprocess.SubprocessError): try: process.kill() - except: - pass + except OSError as kill_err: + logger.debug('Could not kill log stream process: %s', kill_err) thread = threading.Thread(target=stream_logs, daemon=True) thread.start() @@ -836,11 +840,11 @@ def stop_container_log_stream(sid: str): try: process.terminate() process.wait(timeout=2) - except: + except (OSError, subprocess.SubprocessError): try: process.kill() - except: - pass + except OSError as kill_err: + logger.debug('Could not kill log stream process: %s', kill_err) def emit_container_log(app_id: int, line: str, level: str = 'info'): diff --git a/backend/tests/test_servers_list_metrics.py b/backend/tests/test_servers_list_metrics.py index d6010bb5..8d72627c 100644 --- a/backend/tests/test_servers_list_metrics.py +++ b/backend/tests/test_servers_list_metrics.py @@ -108,3 +108,17 @@ def test_list_servers_metrics_are_batched_not_per_row(client, auth_headers, serv assert count_metric_queries() == baseline, ( 'metrics lookup grew with the server count — it must stay batched' ) + + +@pytest.mark.parametrize('param', ['from', 'to']) +def test_metrics_rejects_a_malformed_timestamp_with_400(app, client, auth_headers, param): + with app.app_context(): + server = _mk_server('delta') + db.session.commit() + server_id = server.id + + resp = client.get(f'/api/v1/servers/{server_id}/metrics?{param}=not-a-date', + headers=auth_headers) + + assert resp.status_code == 400 + assert resp.get_json()['error']