security: address PR review - timing-safe comparison, thread lock, specific exceptions, graceful termination

Author: GaneshPatil7517

fri/server/main.py

@@ -6,10 +6,15 @@
 from subprocess import call,check_output
 from pathlib import Path
 import json
+import logging
 import platform
 import re
+import secrets
+import threading
 from flask_cors import CORS, cross_origin
 
+logger = logging.getLogger(__name__)
+
 # Input validation pattern for safe names (alphanumeric, dash, underscore, slash, dot, space)
 SAFE_INPUT_PATTERN = re.compile(r'^[a-zA-Z0-9_\-/. ]+$')
 # Pattern for filenames - no path separators or .. allowed
@@ -91,12 +96,13 @@ def require_api_key():
     """Require a valid API key via X-API-KEY header."""
     if not API_KEY:
         abort(500, description="Server not configured with API key")
-    provided = request.headers.get("X-API-KEY")
-    if provided != API_KEY:
+    provided = request.headers.get("X-API-KEY") or ""
+    if not secrets.compare_digest(provided, API_KEY):
         abort(403, description="Unauthorized")
 
 # Track single Jupyter process to prevent multiple concurrent launches
 jupyter_process = None
+jupyter_lock = threading.Lock()
 
 
 app = Flask(__name__)
@@ -554,20 +560,22 @@ def openJupyter():
 
     require_api_key()
 
-    if jupyter_process and jupyter_process.poll() is None:
-        return jsonify({"message": "Jupyter already running"}), 409
+    with jupyter_lock:
+        if jupyter_process and jupyter_process.poll() is None:
+            return jsonify({"message": "Jupyter already running"}), 409
 
-    try:
-        jupyter_process = subprocess.Popen(
-            ['jupyter', 'lab', '--no-browser'],
-            shell=False,
-            cwd=concore_path,
-            stdout=subprocess.DEVNULL,
-            stderr=subprocess.DEVNULL
-        )
-        return jsonify({"message": "Jupyter Lab started"}), 200
-    except Exception:
-        return jsonify({"error": "Failed to start Jupyter"}), 500
+        try:
+            jupyter_process = subprocess.Popen(
+                ['jupyter', 'lab', '--no-browser'],
+                shell=False,
+                cwd=concore_path,
+                stdout=subprocess.DEVNULL,
+                stderr=subprocess.DEVNULL
+            )
+            return jsonify({"message": "Jupyter Lab started"}), 200
+        except (FileNotFoundError, PermissionError, OSError) as e:
+            logger.error("Failed to start Jupyter: %s", e)
+            return jsonify({"error": "Failed to start Jupyter"}), 500
 
 
 @app.route('/stopJupyter/', methods=['POST'])
@@ -576,13 +584,22 @@ def stopJupyter():
 
     require_api_key()
 
-    if not jupyter_process or jupyter_process.poll() is not None:
-        return jsonify({"message": "No running Jupyter instance"}), 404
-
-    jupyter_process.terminate()
-    jupyter_process = None
+    with jupyter_lock:
+        if not jupyter_process or jupyter_process.poll() is not None:
+            return jsonify({"message": "No running Jupyter instance"}), 404
 
-    return jsonify({"message": "Jupyter stopped"}), 200
+        try:
+            jupyter_process.terminate()
+            # Wait for Jupyter to terminate gracefully
+            jupyter_process.wait(timeout=5)
+        except subprocess.TimeoutExpired:
+            # Force kill if it did not exit in time
+            jupyter_process.kill()
+            jupyter_process.wait(timeout=5)
+        finally:
+            jupyter_process = None
+
+        return jsonify({"message": "Jupyter stopped"}), 200
 
 
 if __name__ == "__main__":

tests/test_openjupyter_security.py

@@ -109,7 +109,7 @@ def test_duplicate_launch_returns_409(self, mock_popen, client):
         data = resp2.get_json()
         assert data["message"] == "Jupyter already running"
 
-    @patch("fri.server.main.subprocess.Popen", side_effect=Exception("fail"))
+    @patch("fri.server.main.subprocess.Popen", side_effect=OSError("fail"))
     def test_popen_failure_returns_500(self, mock_popen, client):
         """If Popen raises, return 500."""
         resp = client.post(
@@ -153,3 +153,4 @@ def test_stop_running_process_returns_200(self, mock_popen, client):
         data = resp.get_json()
         assert data["message"] == "Jupyter stopped"
         mock_proc.terminate.assert_called_once()
+        mock_proc.wait.assert_called()