.Net: Add deny-by-default AllowedUploadDirectories to CloudDrivePlugin

dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs

@@ -1,8 +1,10 @@
 // Copyright (c) Microsoft. All rights reserved.
 
 using System;
+using System.Collections.Generic;
 using System.ComponentModel;
 using System.IO;
+using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.Extensions.Logging;
@@ -14,10 +16,21 @@ namespace Microsoft.SemanticKernel.Plugins.MsGraph;
 /// <summary>
 /// Cloud drive plugin (e.g. OneDrive).
 /// </summary>
+/// <remarks>
+/// <para>
+/// This plugin is secure by default. <see cref="AllowedUploadDirectories"/> must be explicitly configured
+/// before any file upload operations are permitted. By default, all local file paths are denied.
+/// </para>
+/// <para>
+/// When exposing this plugin to an LLM via auto function calling, ensure that
+/// <see cref="AllowedUploadDirectories"/> is restricted to trusted values only.
+/// </para>
+/// </remarks>
 public sealed class CloudDrivePlugin
 {
     private readonly ICloudDriveConnector _connector;
     private readonly ILogger _logger;
+    private HashSet<string> _allowedUploadDirectories = [];
 
     /// <summary>
     /// Initializes a new instance of the <see cref="CloudDrivePlugin"/> class.
@@ -32,6 +45,20 @@ public CloudDrivePlugin(ICloudDriveConnector connector, ILoggerFactory? loggerFa
         this._logger = loggerFactory?.CreateLogger(typeof(CloudDrivePlugin)) ?? NullLogger.Instance;
     }
 
+    /// <summary>
+    /// List of allowed local directories from which files may be uploaded. Subdirectories of allowed directories are also permitted.
+    /// </summary>
+    /// <remarks>
+    /// Defaults to an empty collection (no directories allowed). Must be explicitly populated
+    /// with trusted directory paths before any file upload operations will succeed.
+    /// Paths are canonicalized before validation to prevent directory traversal.
+    /// </remarks>
+    public IEnumerable<string> AllowedUploadDirectories
+    {
+        get => this._allowedUploadDirectories;
+        set => this._allowedUploadDirectories = value is null ? [] : new HashSet<string>(value, StringComparer.OrdinalIgnoreCase);
+    }
+
     /// <summary>
     /// Get the contents of a file stored in a cloud drive.
     /// </summary>
@@ -77,10 +104,19 @@ public async Task UploadFileAsync(
             throw new ArgumentException("Variable was null or whitespace", nameof(destinationPath));
         }
 
-        this._logger.LogDebug("Uploading file '{0}'", filePath);
+        Ensure.NotNullOrWhitespace(filePath, nameof(filePath));
+
+        var canonicalPath = Path.GetFullPath(Environment.ExpandEnvironmentVariables(filePath));
+
+        if (!this.IsUploadPathAllowed(canonicalPath))
+        {
+            throw new InvalidOperationException("Uploading from the provided location is not allowed. Configure 'AllowedUploadDirectories' with trusted directory paths to enable uploads.");
+        }
+
+        this._logger.LogDebug("Uploading file '{0}'", canonicalPath);
 
         // TODO Add support for large file uploads (i.e. upload sessions)
-        await this._connector.UploadSmallFileAsync(filePath, destinationPath, cancellationToken).ConfigureAwait(false);
+        await this._connector.UploadSmallFileAsync(canonicalPath, destinationPath, cancellationToken).ConfigureAwait(false);
     }
 
     /// <summary>
@@ -100,4 +136,59 @@ public async Task<string> CreateLinkAsync(
 
         return await this._connector.CreateShareLinkAsync(filePath, Type, Scope, cancellationToken).ConfigureAwait(false);
     }
+
+    #region private
+    // Use case-insensitive comparison on Windows (case-insensitive FS), case-sensitive on Linux/macOS.
+    private static readonly StringComparison s_pathComparison =
+        RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
+            ? StringComparison.OrdinalIgnoreCase
+            : StringComparison.Ordinal;
+
+    /// <summary>
+    /// If a list of allowed upload directories has been provided, the directory of the provided filePath is checked
+    /// to verify it is in the allowed directory list. Paths are canonicalized before comparison.
+    /// Subdirectories of allowed directories are also permitted.
+    /// </summary>
+    private bool IsUploadPathAllowed(string path)
+    {
+        Ensure.NotNullOrWhitespace(path, nameof(path));
+
+        if (path.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase))
+        {
+            throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
+        }
+
+        string? directoryPath = Path.GetDirectoryName(path);
+
+        if (string.IsNullOrEmpty(directoryPath))
+        {
+            throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(path));
+        }
+
+        if (this._allowedUploadDirectories.Count == 0)
+        {
+            return false;
+        }
+
+        var canonicalDir = Path.GetFullPath(directoryPath);
+
+        foreach (var allowedDirectory in this._allowedUploadDirectories)
+        {
+            var canonicalAllowed = Path.GetFullPath(allowedDirectory);
+            var separator = Path.DirectorySeparatorChar.ToString();
+            if (!canonicalAllowed.EndsWith(separator, s_pathComparison))
+            {
+                canonicalAllowed += separator;
+            }
+
+            if (canonicalDir.StartsWith(canonicalAllowed, s_pathComparison)
+                || (canonicalDir + separator).Equals(canonicalAllowed, s_pathComparison))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
+    #endregion
 }

dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs

@@ -2,6 +2,7 @@
 
 using System;
 using System.IO;
+using System.Runtime.InteropServices;
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
@@ -17,13 +18,14 @@ public class CloudDrivePluginTests
     public async Task UploadSmallFileAsyncSucceedsAsync()
     {
         // Arrange
-        string anyFilePath = Guid.NewGuid().ToString();
+        string allowedDir = Path.GetTempPath();
+        string anyFilePath = Path.Combine(allowedDir, Guid.NewGuid().ToString());
 
         Mock<ICloudDriveConnector> connectorMock = new();
         connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
             .Returns(Task.CompletedTask);
 
-        CloudDrivePlugin target = new(connectorMock.Object);
+        CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
 
         // Act
         await target.UploadFileAsync(anyFilePath, Guid.NewGuid().ToString());
@@ -74,4 +76,164 @@ public async Task GetFileContentAsyncSucceedsAsync()
         Assert.Equal(expectedContent, actual);
         connectorMock.VerifyAll();
     }
+
+    [Fact]
+    public async Task ItDeniesAllPathsByDefaultAsync()
+    {
+        // Arrange
+        string filePath = Path.Combine(Path.GetTempPath(), "somefile.txt");
+
+        Mock<ICloudDriveConnector> connectorMock = new();
+        CloudDrivePlugin target = new(connectorMock.Object);
+
+        // Act & Assert — default config denies all paths
+        await Assert.ThrowsAsync<InvalidOperationException>(async () =>
+            await target.UploadFileAsync(filePath, "/remote.txt"));
+    }
+
+    [Fact]
+    public async Task ItDeniesPathTraversalAsync()
+    {
+        // Arrange
+        var allowedDir = Path.Combine(Path.GetTempPath(), "allowed-folder");
+        var traversalPath = Path.Combine(allowedDir, "..", "outside-folder", "secret.txt");
+
+        Mock<ICloudDriveConnector> connectorMock = new();
+        CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
+
+        // Act & Assert — traversal path is canonicalized and rejected
+        await Assert.ThrowsAsync<InvalidOperationException>(async () =>
+            await target.UploadFileAsync(traversalPath, "/remote.txt"));
+    }
+
+    [Fact]
+    public async Task ItDeniesUncPathsAsync()
+    {
+        // Arrange
+        Mock<ICloudDriveConnector> connectorMock = new();
+        CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [Path.GetTempPath()] };
+
+        // Act & Assert — UNC paths are rejected (ArgumentException on Windows, InvalidOperationException on Linux
+        // where the path is canonicalized differently and fails the allowlist check instead)
+        await Assert.ThrowsAnyAsync<Exception>(async () =>
+            await target.UploadFileAsync("\\\\UNC\\server\\folder\\file.txt", "/remote.txt"));
+    }
+
+    [Fact]
+    public async Task ItDeniesDisallowedDirectoriesAsync()
+    {
+        // Arrange
+        var allowedDir = Path.Combine(Path.GetTempPath(), "allowed");
+        var disallowedPath = Path.Combine(Path.GetTempPath(), "disallowed", "file.txt");
+
+        Mock<ICloudDriveConnector> connectorMock = new();
+        CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
+
+        // Act & Assert
+        await Assert.ThrowsAsync<InvalidOperationException>(async () =>
+            await target.UploadFileAsync(disallowedPath, "/remote.txt"));
+    }
+
+    [Fact]
+    public async Task ItAllowsSubdirectoriesOfAllowedDirectoriesAsync()
+    {
+        // Arrange
+        var allowedDir = Path.GetTempPath();
+        var subDirPath = Path.Combine(allowedDir, "subdir", "nested", "file.txt");
+
+        Mock<ICloudDriveConnector> connectorMock = new();
+        connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
+            .Returns(Task.CompletedTask);
+
+        CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
+
+        // Act — subdirectory of allowed folder should succeed
+        await target.UploadFileAsync(subDirPath, "/remote.txt");
+
+        // Assert
+        connectorMock.VerifyAll();
+    }
+
+    [Fact]
+    public async Task ItExpandsEnvironmentVariablesAndValidatesAsync()
+    {
+        // Arrange — set a dedicated test env var to avoid platform-specific assumptions
+        var tempDir = Path.GetTempPath().TrimEnd(Path.DirectorySeparatorChar);
+        var envVarName = "SK_TEST_UPLOAD_DIR";
+        var originalValue = Environment.GetEnvironmentVariable(envVarName);
+        try
+        {
+            Environment.SetEnvironmentVariable(envVarName, tempDir);
+            var envVarPath = Path.Combine($"%{envVarName}%", "testfile.txt");
+
+            Mock<ICloudDriveConnector> connectorMock = new();
+            connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
+                .Returns(Task.CompletedTask);
+
+            CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [tempDir] };
+
+            // Act — env var should be expanded and path should be allowed
+            await target.UploadFileAsync(envVarPath, "/remote.txt");
+
+            // Assert
+            connectorMock.VerifyAll();
+        }
+        finally
+        {
+            Environment.SetEnvironmentVariable(envVarName, originalValue);
+        }
+    }
+
+    [Fact]
+    public async Task ItDeniesExpandedEnvironmentVariablePathsOutsideAllowedAsync()
+    {
+        // Arrange — set a dedicated test env var; allow a subdirectory but env var expands outside it
+        var tempDir = Path.GetTempPath().TrimEnd(Path.DirectorySeparatorChar);
+        var allowedDir = Path.Combine(tempDir, "specific-allowed");
+        var envVarName = "SK_TEST_UPLOAD_DIR";
+        var originalValue = Environment.GetEnvironmentVariable(envVarName);
+        try
+        {
+            Environment.SetEnvironmentVariable(envVarName, tempDir);
+            var envVarPath = Path.Combine($"%{envVarName}%", "outside-file.txt");
+
+            Mock<ICloudDriveConnector> connectorMock = new();
+            CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
+
+            // Act & Assert — expanded path is outside allowed directory
+            await Assert.ThrowsAsync<InvalidOperationException>(async () =>
+                await target.UploadFileAsync(envVarPath, "/remote.txt"));
+        }
+        finally
+        {
+            Environment.SetEnvironmentVariable(envVarName, originalValue);
+        }
+    }
+
+    [Fact]
+    public async Task ItRespectsPlatformCaseSensitivityAsync()
+    {
+        // Arrange — use differently-cased allowed dir vs file path
+        var allowedDir = Path.Combine(Path.GetTempPath(), "AllowedFolder");
+        var filePath = Path.Combine(Path.GetTempPath(), "allowedfolder", "file.txt");
+
+        Mock<ICloudDriveConnector> connectorMock = new();
+        connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
+            .Returns(Task.CompletedTask);
+
+        CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
+
+        if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+        {
+            // Windows: case-insensitive FS — differently-cased path should be allowed
+            await target.UploadFileAsync(filePath, "/remote.txt");
+            connectorMock.VerifyAll();
+        }
+        else
+        {
+            // Linux/macOS: case-sensitive FS — differently-cased path should be denied
+            await Assert.ThrowsAsync<InvalidOperationException>(async () =>
+                await target.UploadFileAsync(filePath, "/remote.txt"));
+        }
+    }
 }