ADR-006: Profile Manager Security & Performance Patterns

Status

Proposed

Context

During LLM Council verification of v4.10.0, critical security and performance issues were identified in conductor-gui/src-tauri/src/profile_manager.rs:

Security Issues (P0)

1. Arbitrary file read: import_profile_json() accepts untrusted config_path without validation, allowing reads of /etc/passwd
2. Path traversal: import_profile_toml() uses filename directly as profile ID, allowing ../../../etc/passwd.toml to write anywhere
3. TOCTOU race conditions: update_profile() and delete_profile() release locks between check and use, causing data loss under concurrency

Performance Issues (P1)

4. Blocking I/O in async: 8 locations use std::fs operations inside async functions, blocking the Tokio runtime and causing GUI timeouts
5. Cache staleness: load_profile_config() only uses TTL-based caching, missing external file modifications
6. Non-deterministic default: get_default_profile() uses HashMap::values().find() which has undefined iteration order

Decision

Establish security and performance patterns for ProfileManager:

1. Path Validation Module

Create dedicated path_validation.rs module with:

/// Reject profile IDs with path separators, traversal, null bytes
pub fn validate_profile_id(id: &str) -> Result<(), PathValidationError>

/// Validate config paths are within allowed directories after canonicalization
pub fn validate_config_path(path: &Path, profiles_dir: &Path) -> Result<PathBuf, PathValidationError>

/// Construct safe destination paths with validation
pub fn safe_profile_destination(profiles_dir: &Path, id: &str, ext: &str) -> Result<PathBuf, PathValidationError>

/// Sanitize untrusted IDs by removing dangerous characters
pub fn sanitize_profile_id(raw: &str) -> String

Validation rules:

• Reject empty IDs
• Reject IDs containing /, \, .., null bytes
• Reject IDs starting with . (hidden files)
• Canonicalize paths to resolve symlinks before checking
• Only allow paths within profiles_dir or user config directory

2. Import Security Pattern

All profile import functions must validate paths before use:

pub async fn import_profile_json(&self, json: &str) -> Result<String, String> {
    let profile: AppProfile = serde_json::from_str(json)?;

    // SECURITY: Validate before any file operations
    validate_config_path(&profile.config_path, &self.profiles_dir)?;

    self.register_profile(profile).await
}

pub async fn import_profile_toml(&self, source: &Path) -> Result<String, String> {
    // SECURITY: Sanitize filename-derived profile ID
    let raw_id = source.file_stem()...;
    let profile_id = sanitize_profile_id(&raw_id);
    validate_profile_id(&profile_id)?;

    let dest = safe_profile_destination(&self.profiles_dir, &profile_id, "toml")?;
    // Safe to proceed
}

3. Atomic Read-Modify-Write Pattern

Following ADR-003 Pattern 4, hold all required locks for the duration of operations:

Bad (TOCTOU vulnerable):

pub async fn delete_profile(&self, profile_id: &str) -> Result<(), String> {
    let profile = self.get_profile(profile_id).await?; // Read lock released
    // RACE WINDOW
    let mut bundle_map = self.bundle_map.write().await;
    // Profile may have changed or been deleted by another task
}

Good (Atomic):

pub async fn delete_profile(&self, profile_id: &str) -> Result<(), String> {
    // Hold all locks for entire operation
    let mut profiles = self.profiles.write().await;
    let mut bundle_map = self.bundle_map.write().await;

    let profile = profiles.remove(profile_id).ok_or("Profile not found")?;

    // Safe: no other task can modify state while we hold both locks
    for bundle_id in &profile.bundle_ids {
        if let Some(mapped_id) = bundle_map.get(bundle_id) {
            if mapped_id == profile_id {
                bundle_map.remove(bundle_id);
            }
        }
    }
    // Locks released at end of scope
}

4. Async I/O Pattern

Use tokio::fs instead of std::fs in all async functions:

Blocking	Non-blocking
std::fs::read_to_string()	tokio::fs::read_to_string().await
std::fs::write()	tokio::fs::write().await
std::fs::read_dir()	tokio::fs::read_dir().await
std::fs::metadata()	tokio::fs::metadata().await
std::fs::create_dir_all()	tokio::fs::create_dir_all().await
std::fs::copy()	tokio::fs::copy().await

Rationale: Blocking I/O on the Tokio runtime thread blocks all async tasks, causing GUI freezes and timeouts.

5. Mtime-Based Cache Invalidation

Track file modification time in addition to TTL:

struct CachedProfile {
    profile: AppProfile,
    config_content: String,
    cached_at: Instant,
    file_mtime: Option<std::time::SystemTime>, // Track file modification
}

pub async fn load_profile_config(&self, profile_id: &str) -> Result<String, String> {
    // Check BOTH TTL and file mtime
    if cache_entry.age < TTL {
        let current_mtime = tokio::fs::metadata(&path).await?.modified()?;
        if current_mtime <= cache_entry.file_mtime {
            return Ok(cache_entry.config_content.clone()); // Serve from cache
        }
    }
    // File modified or TTL expired - reload from disk
}

6. Deterministic Collection Iteration

When selecting from collections with undefined iteration order, sort first:

pub async fn get_default_profile(&self) -> Option<AppProfile> {
    let defaults: Vec<_> = profiles.values().filter(|p| p.is_default).collect();

    match defaults.len() {
        0 => None,
        1 => Some(defaults[0].clone()),
        _ => {
            tracing::warn!("Multiple default profiles found");
            // Return alphabetically first for determinism
            defaults.into_iter().min_by(|a, b| a.id.cmp(&b.id)).cloned()
        }
    }
}

Consequences

Positive

• Prevents arbitrary file read/write vulnerabilities
• Eliminates path traversal attacks
• No data loss from concurrent operations
• No GUI freezes from blocking I/O
• Cache reflects external file changes
• Predictable behavior even with invariant violations

Negative

• More verbose validation code
• Slight overhead from path canonicalization
• Requires async-aware constructors (or spawn_blocking)

Implementation

Files Modified

• conductor-gui/src-tauri/src/path_validation.rs (NEW)
• conductor-gui/src-tauri/src/profile_manager.rs
• conductor-gui/src-tauri/src/lib.rs
• conductor-gui/src-tauri/src/main.rs

TDD Test Coverage

// Path validation (18 tests)
#[test] fn test_validate_profile_id_rejects_traversal()
#[test] fn test_validate_config_path_rejects_etc_passwd()
#[test] fn test_validate_config_path_rejects_symlink_to_forbidden()
#[test] fn test_safe_profile_destination_rejects_traversal()

// Security fixes (4 tests)
#[tokio::test] async fn test_import_profile_json_rejects_etc_passwd()
#[tokio::test] async fn test_import_profile_toml_sanitizes_filename()

// Concurrency fixes (2 tests)
#[tokio::test] async fn test_concurrent_updates_no_data_loss()
#[tokio::test] async fn test_concurrent_delete_and_update()

// Cache and determinism (2 tests)
#[tokio::test] async fn test_cache_invalidates_on_file_modification()
#[tokio::test] async fn test_deterministic_default_profile_selection()

v4.10.3 Security Hardening

Following LLM Council verification of v4.10.2, additional security measures were implemented:

7. File Permission Hardening (Unix)

Created config files must have restrictive permissions (0o600) to prevent unauthorized access regardless of system umask:

// After file.write_all()
#[cfg(unix)]
{
    use std::os::unix::fs::PermissionsExt;
    let perms = std::fs::Permissions::from_mode(0o600);
    tokio::fs::set_permissions(path, perms).await?;
}

Test: test_created_profile_config_has_restrictive_permissions

8. Profile Name Validation

Display names in update_profile() must be validated to prevent:

• Log injection via control characters
• Memory exhaustion via long strings
• Null byte injection

pub fn validate_profile_name(name: &str) -> Result<(), PathValidationError> {
    // Reject empty names
    // Reject names > 256 chars
    // Reject null bytes
    // Reject control characters (except space/tab)
}

Tests: test_validate_profile_name_*, test_update_profile_rejects_invalid_name

9. Bounded Main Config Read

When auto-generating profile configs, limit main_config file read to 10MB to prevent memory exhaustion DoS:

const MAX_MAIN_CONFIG_SIZE: u64 = 10 * 1024 * 1024;

let content = if main_config.exists() {
    let metadata = tokio::fs::metadata(&main_config).await.ok();
    let size = metadata.as_ref().map(|m| m.len()).unwrap_or(0);
    if size > MAX_MAIN_CONFIG_SIZE {
        // Fall back to default template
        include_str!("../resources/default_profile.toml").to_string()
    } else {
        tokio::fs::read_to_string(&main_config).await.unwrap_or_else(...)
    }
} else {
    include_str!("../resources/default_profile.toml").to_string()
};

10. Filename Collision Detection

When auto-generating config paths, detect existing files and add numeric suffix to prevent profile overwrite:

let mut suffix = 1;
const MAX_COLLISION_ATTEMPTS: u32 = 1000;
while config_path.exists() {
    config_filename = format!("{}-{}.toml", sanitized_id, suffix);
    config_path = self.profiles_dir.join(&config_filename);
    suffix += 1;
    if suffix > MAX_COLLISION_ATTEMPTS {
        return Err("Too many profile filename collisions");
    }
}

Tests: test_register_profile_handles_filename_collision, test_register_profile_handles_multiple_collisions

v4.10.3 TDD Test Coverage

// File permissions (1 test)
#[tokio::test] #[cfg(unix)]
async fn test_created_profile_config_has_restrictive_permissions()

// Name validation (5 tests)
#[test] fn test_validate_profile_name_accepts_valid()
#[test] fn test_validate_profile_name_rejects_empty()
#[test] fn test_validate_profile_name_rejects_control_chars()
#[test] fn test_validate_profile_name_rejects_too_long()
#[tokio::test] async fn test_update_profile_rejects_invalid_name()

// Collision detection (2 tests)
#[tokio::test] async fn test_register_profile_handles_filename_collision()
#[tokio::test] async fn test_register_profile_handles_multiple_collisions()

References

• GitHub Epic: TBD
• GitHub Issues: #55-58 (v4.10.3 security tracking)
• LLM Council v4.10.0, v4.10.2 verification findings
• ADR-003: GUI State Management Patterns
• conductor-core/src/config/loader.rs (path validation patterns)