Path Canonicalization for Safe Cleanup Operations

Path Canonicalization for Safe Cleanup Operations

Problem

Cleanup or rollback operations that delete files based on a list of paths can be exploited to delete files outside the intended directory. This occurs when:

1. The file list contains relative paths with .. traversal
2. Symlinks point outside the target directory
3. Race conditions allow path substitution between validation and deletion

Symptoms:

• Files deleted outside workspace during rollback
• Security audit flags “unsafe file deletion”
• Data loss when symlinks are present in import source

Investigation

Steps Tried

1. String prefix checking - Failed because "/workspace/foo/../../../etc" starts with "/workspace" but resolves elsewhere
2. Path component filtering - Partially worked but missed symlink resolution
3. Canonicalization of both paths - Correct solution

Root Cause

Path string manipulation doesn’t account for:

• Symbolic links that resolve to external locations
• Redundant separators and ./.. components
• Platform-specific path normalization differences

Solution

Canonicalize both the workspace root AND each file path, then verify containment:

Code Changes

// Before (vulnerable)
fn rollback_import(workspace_path: &Path, imported_files: &[PathBuf]) {
    for relative_path in imported_files {
        let full_path = workspace_path.join(relative_path);
        if full_path.exists() {
            fs::remove_file(&full_path).ok();  // UNSAFE!
        }
    }
}

// After (safe)
fn rollback_import(workspace_path: &Path, imported_files: &[PathBuf]) {
    // Canonicalize workspace ONCE at the start
    let canonical_workspace = match workspace_path.canonicalize() {
        Ok(p) => p,
        Err(e) => {
            tracing::warn!("Cannot canonicalize workspace for rollback: {}", e);
            return;
        }
    };

    for relative_path in imported_files {
        let full_path = workspace_path.join(relative_path);

        // Verify path is within workspace AFTER canonicalization
        match full_path.canonicalize() {
            Ok(canonical_file) => {
                if !canonical_file.starts_with(&canonical_workspace) {
                    tracing::warn!(
                        "Rollback: refusing to delete file outside workspace: {:?}",
                        full_path
                    );
                    continue;
                }

                // Safe to delete - verified within workspace
                if let Err(e) = fs::remove_file(&canonical_file) {
                    tracing::debug!("Failed to remove file during rollback: {}", e);
                }
            }
            Err(_) => {
                // File doesn't exist or can't be resolved - skip
                continue;
            }
        }
    }
}

Implementation Notes

• Canonicalize workspace path once at function start (efficiency)
• Canonicalize each file path individually (correctness)
• Use starts_with on canonicalized paths, not string prefix
• Log warnings for skipped files (audit trail)
• Handle canonicalization failures gracefully (file may not exist)

Prevention

Best Practices

1. Always canonicalize both paths before containment checks
2. Fail closed - if canonicalization fails, don’t delete
3. Log skipped operations for security auditing
4. Validate paths at entry points (see path_validation.rs)

Warning Signs

• Any cleanup code using join() + exists() + remove_file() without canonicalization
• String-based path prefix checks (path.starts_with("/safe/dir"))
• Rollback operations that iterate over user-provided paths

Related Tests

#[test]
fn test_rollback_rejects_traversal_attempt() {
    let workspace = TempDir::new().unwrap();
    let external_file = TempDir::new().unwrap().path().join("secret.txt");
    fs::write(&external_file, "sensitive data").unwrap();

    // Attempt traversal via relative path
    let malicious_paths = vec![
        PathBuf::from("../../../").join(external_file.file_name().unwrap())
    ];

    rollback_import(workspace.path(), &malicious_paths);

    // External file should NOT be deleted
    assert!(external_file.exists(), "Traversal attack should be blocked");
}

#[test]
fn test_rollback_rejects_symlink_escape() {
    let workspace = TempDir::new().unwrap();
    let external_dir = TempDir::new().unwrap();
    let external_file = external_dir.path().join("secret.txt");
    fs::write(&external_file, "sensitive data").unwrap();

    // Create symlink inside workspace pointing outside
    let symlink_path = workspace.path().join("escape");
    std::os::unix::fs::symlink(external_dir.path(), &symlink_path).unwrap();

    let malicious_paths = vec![PathBuf::from("escape/secret.txt")];

    rollback_import(workspace.path(), &malicious_paths);

    // External file should NOT be deleted
    assert!(external_file.exists(), "Symlink escape should be blocked");
}

References

• OWASP Path Traversal
• Commit: 52bf3bf - feat(import): add Copy and In-Place import modes
• Related: crates/infrastructure/import/src/import/path_validation.rs