Address all code review comments from PR #3

Co-authored-by: Steake <530040+Steake@users.noreply.github.com>

Author: Copilot

crates/bitcell-admin/README.md

@@ -162,14 +162,23 @@ bitcell-admin/
 
 ## Security Considerations
 
-⚠️ **IMPORTANT**: The admin console provides powerful administrative capabilities. In production:
-
-1. **Enable authentication** before exposing to network
-2. **Use HTTPS/TLS** for encrypted communication
-3. **Restrict access** via firewall rules or VPN
-4. **Use strong passwords** and rotate regularly
-5. **Enable audit logging** for all administrative actions
-6. **Limit API rate limits** to prevent abuse
+⚠️ **CRITICAL SECURITY WARNING** ⚠️
+
+**NO AUTHENTICATION IS CURRENTLY IMPLEMENTED**
+
+The admin console currently allows **unrestricted access** to all endpoints. This is a **critical security vulnerability**.
+
+**DO NOT expose this admin console to any network (including localhost) in production without implementing authentication first.**
+
+For production deployments, you MUST:
+
+1. **Implement authentication** before exposing to any network
+2. **Use HTTPS/TLS** for all communication (never HTTP in production)
+3. **Restrict network access** via firewall rules, VPN, or IP allowlisting
+4. **Use strong passwords** and rotate them regularly
+5. **Enable comprehensive audit logging** for all administrative actions
+6. **Implement API rate limiting** to prevent abuse
+7. **Run with least-privilege** user accounts (never as root)
 
 ## Development
 

crates/bitcell-admin/src/api/metrics.rs

@@ -78,13 +78,9 @@ pub async fn get_metrics(
         .await
         .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(e)))?;
 
-    // Calculate system metrics (placeholder - would normally come from node metrics or system stats)
-    let uptime_seconds = if let Some(first_node) = aggregated.node_metrics.first() {
-        let duration = chrono::Utc::now().signed_duration_since(first_node.last_updated);
-        duration.num_seconds().max(0) as u64
-    } else {
-        0
-    };
+    // Calculate system metrics
+    // TODO: Track actual node start times to compute real uptime
+    let uptime_seconds = 0u64; // Placeholder - requires node start time tracking
 
     let response = MetricsResponse {
         chain: ChainMetrics {
@@ -100,20 +96,20 @@ pub async fn get_metrics(
             total_peers: aggregated.total_nodes * 10, // Estimate
             bytes_sent: aggregated.bytes_sent,
             bytes_received: aggregated.bytes_received,
-            messages_sent: 0, // TODO: Add to node metrics
-            messages_received: 0, // TODO: Add to node metrics
+            messages_sent: 0, // TODO: Requires adding message_sent to node metrics
+            messages_received: 0, // TODO: Requires adding message_received to node metrics
         },
         ebsl: EbslMetrics {
             active_miners: aggregated.active_miners,
             banned_miners: aggregated.banned_miners,
-            average_trust_score: 0.85, // TODO: Calculate from actual data
-            total_slashing_events: 0, // TODO: Add to node metrics
+            average_trust_score: 0.85, // TODO: Requires adding trust scores to node metrics
+            total_slashing_events: 0, // TODO: Requires adding slashing events to node metrics
         },
         system: SystemMetrics {
             uptime_seconds,
-            cpu_usage: 0.0, // TODO: Add system metrics collection
-            memory_usage_mb: 0, // TODO: Add system metrics collection
-            disk_usage_mb: 0, // TODO: Add system metrics collection
+            cpu_usage: 0.0, // TODO: Requires system metrics collection (e.g., sysinfo crate)
+            memory_usage_mb: 0, // TODO: Requires system metrics collection
+            disk_usage_mb: 0, // TODO: Requires system metrics collection
         },
     };
 

crates/bitcell-admin/src/api/nodes.rs

@@ -47,6 +47,16 @@ pub async fn get_node(
     State(state): State<Arc<AppState>>,
     Path(id): Path<String>,
 ) -> Result<Json<NodeResponse>, (StatusCode, Json<ErrorResponse>)> {
+    // Validate node ID format (alphanumeric and hyphens only)
+    if !id.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_') {
+        return Err((
+            StatusCode::BAD_REQUEST,
+            Json(ErrorResponse {
+                error: "Invalid node ID format".to_string(),
+            }),
+        ));
+    }
+
     match state.process.get_node(&id) {
         Some(node) => Ok(Json(NodeResponse { node })),
         None => Err((
@@ -62,8 +72,28 @@ pub async fn get_node(
 pub async fn start_node(
     State(state): State<Arc<AppState>>,
     Path(id): Path<String>,
-    Json(_req): Json<StartNodeRequest>,
+    Json(req): Json<StartNodeRequest>,
 ) -> Result<Json<NodeResponse>, (StatusCode, Json<ErrorResponse>)> {
+    // Validate node ID format (alphanumeric and hyphens only)
+    if !id.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_') {
+        return Err((
+            StatusCode::BAD_REQUEST,
+            Json(ErrorResponse {
+                error: "Invalid node ID format".to_string(),
+            }),
+        ));
+    }
+
+    // Config is not supported yet
+    if req.config.is_some() {
+        return Err((
+            StatusCode::BAD_REQUEST,
+            Json(ErrorResponse {
+                error: "Custom config is not supported yet".to_string(),
+            }),
+        ));
+    }
+
     match state.process.start_node(&id) {
         Ok(node) => {
             tracing::info!("Started node '{}' successfully", id);
@@ -83,6 +113,16 @@ pub async fn stop_node(
     State(state): State<Arc<AppState>>,
     Path(id): Path<String>,
 ) -> Result<Json<NodeResponse>, (StatusCode, Json<ErrorResponse>)> {
+    // Validate node ID format (alphanumeric and hyphens only)
+    if !id.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_') {
+        return Err((
+            StatusCode::BAD_REQUEST,
+            Json(ErrorResponse {
+                error: "Invalid node ID format".to_string(),
+            }),
+        ));
+    }
+
     match state.process.stop_node(&id) {
         Ok(node) => {
             tracing::info!("Stopped node '{}' successfully", id);

crates/bitcell-admin/src/api/setup.rs

@@ -9,7 +9,7 @@ use serde::{Deserialize, Serialize};
 use std::sync::Arc;
 
 use crate::AppState;
-use crate::setup::NodeEndpoint;
+use crate::setup::{NodeEndpoint, SETUP_FILE_PATH};
 
 #[derive(Debug, Serialize)]
 pub struct SetupStatusResponse {
@@ -68,7 +68,7 @@ pub async fn add_node(
     state.setup.add_node(node.clone());
 
     // Save setup state
-    let setup_path = std::path::PathBuf::from(".bitcell/admin/setup.json");
+    let setup_path = std::path::PathBuf::from(SETUP_FILE_PATH);
     state.setup.save_to_file(&setup_path)
         .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(e)))?;
 
@@ -87,7 +87,7 @@ pub async fn set_config_path(
     state.setup.set_config_path(path.clone());
 
     // Save setup state
-    let setup_path = std::path::PathBuf::from(".bitcell/admin/setup.json");
+    let setup_path = std::path::PathBuf::from(SETUP_FILE_PATH);
     state.setup.save_to_file(&setup_path)
         .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(e)))?;
 
@@ -101,17 +101,30 @@ pub async fn set_data_dir(
 ) -> Result<Json<String>, (StatusCode, Json<String>)> {
     let path = std::path::PathBuf::from(&req.path);
 
-    // Create directory if it doesn't exist
+    // Create directory if it doesn't exist with restrictive permissions
     std::fs::create_dir_all(&path)
         .map_err(|e| (
             StatusCode::INTERNAL_SERVER_ERROR,
             Json(format!("Failed to create data directory: {}", e))
         ))?;
 
+    // Set restrictive permissions on Unix systems (0700 - owner only)
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::PermissionsExt;
+        let permissions = std::fs::Permissions::from_mode(0o700);
+        std::fs::set_permissions(&path, permissions)
+            .map_err(|e| (
+                StatusCode::INTERNAL_SERVER_ERROR,
+                Json(format!("Failed to set directory permissions: {}", e))
+            ))?;
+        tracing::info!("Set data directory permissions to 0700 (owner only)");
+    }
+
     state.setup.set_data_dir(path);
 
     // Save setup state
-    let setup_path = std::path::PathBuf::from(".bitcell/admin/setup.json");
+    let setup_path = std::path::PathBuf::from(SETUP_FILE_PATH);
     state.setup.save_to_file(&setup_path)
         .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(e)))?;
 
@@ -125,7 +138,7 @@ pub async fn complete_setup(
     state.setup.mark_initialized();
 
     // Save setup state
-    let setup_path = std::path::PathBuf::from(".bitcell/admin/setup.json");
+    let setup_path = std::path::PathBuf::from(SETUP_FILE_PATH);
     state.setup.save_to_file(&setup_path)
         .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(e)))?;
 

crates/bitcell-admin/src/api/test.rs

@@ -114,18 +114,16 @@ pub async fn run_battle_test(
 
     let (outcome, energy_a, energy_b) = tokio::task::spawn_blocking(move || {
         // Simulate the battle
-        let outcome = battle.simulate()
-            .map_err(|e| format!("Battle simulation error: {:?}", e))?;
+        let outcome = battle.simulate();
 
         // Get final grid to measure energies
         let final_grid = battle.final_grid();
         let (energy_a, energy_b) = battle.measure_regional_energy(&final_grid);
 
-        Ok::<_, String>((outcome, energy_a, energy_b))
+        (outcome, energy_a, energy_b)
     })
     .await
-    .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(format!("Task join error: {}", e))))?
-    .map_err(|e: String| (StatusCode::INTERNAL_SERVER_ERROR, Json(e)))?;
+    .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(format!("Task join error: {}", e))))?;
 
     let duration = start.elapsed();
 
@@ -222,8 +220,7 @@ pub async fn run_battle_visualization(
     // Run simulation and capture frames
     let (outcome, frames) = tokio::task::spawn_blocking(move || {
         // Get outcome
-        let outcome = battle.simulate()
-            .map_err(|e| format!("Battle simulation error: {:?}", e))?;
+        let outcome = battle.simulate();
 
         // Get grid states at sample steps
         let grids = battle.grid_states(&sample_steps);
@@ -243,11 +240,10 @@ pub async fn run_battle_visualization(
             });
         }
 
-        Ok::<_, String>((outcome, frames))
+        (outcome, frames)
     })
     .await
-    .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(format!("Task join error: {}", e))))?
-    .map_err(|e: String| (StatusCode::INTERNAL_SERVER_ERROR, Json(e)))?;
+    .map_err(|e| (StatusCode::INTERNAL_SERVER_ERROR, Json(format!("Task join error: {}", e))))?;
 
     let winner = match outcome {
         BattleOutcome::AWins => "glider_a".to_string(),

crates/bitcell-admin/src/lib.rs

@@ -30,6 +30,7 @@ pub use api::AdminApi;
 pub use deployment::DeploymentManager;
 pub use config::ConfigManager;
 pub use process::ProcessManager;
+pub use setup::SETUP_FILE_PATH;
 
 /// Administrative console server
 pub struct AdminConsole {
@@ -50,7 +51,7 @@ impl AdminConsole {
         let setup = Arc::new(setup::SetupManager::new());
 
         // Try to load setup state from default location
-        let setup_path = std::path::PathBuf::from(".bitcell/admin/setup.json");
+        let setup_path = std::path::PathBuf::from(SETUP_FILE_PATH);
         if let Err(e) = setup.load_from_file(&setup_path) {
             tracing::warn!("Failed to load setup state: {}", e);
         }
@@ -112,7 +113,9 @@ impl AdminConsole {
             // Static files
             .nest_service("/static", ServeDir::new("static"))
 
-            // CORS
+            // CORS - WARNING: Permissive CORS allows requests from any origin.
+            // This is only suitable for local development. For production,
+            // configure specific allowed origins to prevent CSRF attacks.
             .layer(CorsLayer::permissive())
 
             // State

crates/bitcell-admin/src/metrics_client.rs

@@ -31,9 +31,9 @@ impl MetricsClient {
     pub fn new() -> Self {
         Self {
             client: reqwest::Client::builder()
-                .timeout(Duration::from_secs(5))
+                .timeout(Duration::from_secs(10))
                 .build()
-                .unwrap(),
+                .expect("Failed to build HTTP client for metrics"),
         }
     }
 
@@ -58,6 +58,9 @@ impl MetricsClient {
     }
 
     /// Parse Prometheus metrics format
+    /// NOTE: This is a basic parser that only handles simple "metric_name value" format.
+    /// It does NOT support metric labels (e.g., metric{label="value"}).
+    /// For production use, consider using a proper Prometheus parsing library.
     fn parse_prometheus_metrics(&self, node_id: &str, endpoint: &str, text: &str) -> Result<NodeMetrics, String> {
         let mut metrics = HashMap::new();
 

crates/bitcell-admin/src/process.rs

@@ -69,6 +69,8 @@ impl ProcessManager {
         }
 
         // Build command to start node
+        // NOTE: Uses 'cargo run' which is suitable for development only.
+        // For production deployments, use a compiled binary path instead.
         let mut cmd = Command::new("cargo");
         cmd.arg("run")
             .arg("-p")
@@ -93,7 +95,10 @@ impl ProcessManager {
 
         // Spawn the process
         let child = cmd.spawn()
-            .map_err(|e| format!("Failed to spawn process: {}", e))?;
+            .map_err(|e| {
+                tracing::error!("Failed to spawn process for node '{}': {:?}", id, e);
+                "Failed to start node process".to_string()
+            })?;
 
         node.process = Some(child);
         node.info.status = NodeStatus::Running;
@@ -116,10 +121,20 @@ impl ProcessManager {
             // Try graceful shutdown first
             #[cfg(unix)]
             {
-                use std::os::unix::process::CommandExt;
                 let pid = process.id();
-                unsafe {
-                    libc::kill(pid as i32, libc::SIGTERM);
+                // SAFETY: We call libc::kill to send SIGTERM to the child process.
+                // The PID is obtained from process.id(), which should be valid for a running child.
+                // However, the process may have already exited, or permissions may be insufficient.
+                // We check the return value for errors.
+                let res = unsafe { libc::kill(pid as i32, libc::SIGTERM) };
+                if res != 0 {
+                    let errno = std::io::Error::last_os_error();
+                    tracing::warn!(
+                        "Failed to send SIGTERM to process {} for node '{}': {}",
+                        pid,
+                        id,
+                        errno
+                    );
                 }
 
                 // Wait up to 5 seconds for graceful shutdown

crates/bitcell-admin/src/setup.rs

@@ -4,6 +4,9 @@ use serde::{Deserialize, Serialize};
 use std::path::PathBuf;
 use std::sync::RwLock;
 
+/// Default setup file path
+pub const SETUP_FILE_PATH: &str = ".bitcell/admin/setup.json";
+
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub struct SetupState {
     pub initialized: bool,