Address security, performance, and code quality issues from PR #3 review and fix admin console UI

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

@@ -32,6 +32,19 @@ pub struct StartNodeRequest {
     pub config: Option<serde_json::Value>,
 }
 
+/// Validate node ID format (alphanumeric, hyphens, and underscores only)
+fn validate_node_id(id: &str) -> Result<(), (StatusCode, Json<ErrorResponse>)> {
+    if id.is_empty() || !id.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_') {
+        return Err((
+            StatusCode::BAD_REQUEST,
+            Json(ErrorResponse {
+                error: "Invalid node ID format".to_string(),
+            }),
+        ));
+    }
+    Ok(())
+}
+
 /// List all registered nodes
 pub async fn list_nodes(
     State(state): State<Arc<AppState>>,
@@ -47,6 +60,8 @@ pub async fn get_node(
     State(state): State<Arc<AppState>>,
     Path(id): Path<String>,
 ) -> Result<Json<NodeResponse>, (StatusCode, Json<ErrorResponse>)> {
+    validate_node_id(&id)?;
+
     match state.process.get_node(&id) {
         Some(node) => Ok(Json(NodeResponse { node })),
         None => Err((
@@ -62,8 +77,21 @@ 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(&id)?;
+
+    // Config is not supported yet
+    if req.config.is_some() {
+        tracing::warn!("Node '{}': Rejected start request with unsupported config", id);
+        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 +111,8 @@ pub async fn stop_node(
     State(state): State<Arc<AppState>>,
     Path(id): Path<String>,
 ) -> Result<Json<NodeResponse>, (StatusCode, Json<ErrorResponse>)> {
+    validate_node_id(&id)?;
+
     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/main.rs

@@ -1,6 +1,6 @@
 //! BitCell Admin Console - Main Entry Point
 
-use bitcell_admin::{AdminConsole, process::{ProcessManager, NodeConfig}, api::NodeType};
+use bitcell_admin::AdminConsole;
 use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt};
 
 #[tokio::main]
@@ -23,47 +23,11 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
         .parse()?;
 
     let console = AdminConsole::new(addr);
-    let process_mgr = console.process_manager();
 
-    // Register some sample nodes for demonstration
-    register_sample_nodes(&process_mgr);
-
-    tracing::info!("Admin console ready - registered {} nodes", process_mgr.list_nodes().len());
+    tracing::info!("Admin console ready");
     tracing::info!("Dashboard available at http://{}", addr);
 
     console.serve().await?;
 
     Ok(())
 }
-
-fn register_sample_nodes(process: &ProcessManager) {
-    // Register sample validator nodes
-    for i in 1..=3 {
-        let config = NodeConfig {
-            node_type: NodeType::Validator,
-            data_dir: format!("/tmp/bitcell/validator-{}", i),
-            port: 9000 + i as u16,
-            rpc_port: 10000 + i as u16,
-            log_level: "info".to_string(),
-            network: "testnet".to_string(),
-        };
-
-        process.register_node(format!("validator-{}", i), config);
-        tracing::info!("Registered validator-{}", i);
-    }
-
-    // Register sample miner nodes
-    for i in 1..=2 {
-        let config = NodeConfig {
-            node_type: NodeType::Miner,
-            data_dir: format!("/tmp/bitcell/miner-{}", i),
-            port: 9100 + i as u16,
-            rpc_port: 10100 + i as u16,
-            log_level: "info".to_string(),
-            network: "testnet".to_string(),
-        };
-
-        process.register_node(format!("miner-{}", i), config);
-        tracing::info!("Registered miner-{}", i);
-    }
-}

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();