diff --git a/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs b/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs index de8660092fe4..374bdd4ef1de 100644 --- a/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs +++ b/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; /// /// Cloud drive plugin (e.g. OneDrive). /// +/// +/// +/// This plugin is secure by default. must be explicitly configured +/// before any file upload operations are permitted. By default, all local file paths are denied. +/// +/// +/// When exposing this plugin to an LLM via auto function calling, ensure that +/// is restricted to trusted values only. +/// +/// public sealed class CloudDrivePlugin { private readonly ICloudDriveConnector _connector; private readonly ILogger _logger; + private HashSet _allowedUploadDirectories = []; /// /// Initializes a new instance of the class. @@ -32,6 +45,20 @@ public CloudDrivePlugin(ICloudDriveConnector connector, ILoggerFactory? loggerFa this._logger = loggerFactory?.CreateLogger(typeof(CloudDrivePlugin)) ?? NullLogger.Instance; } + /// + /// List of allowed local directories from which files may be uploaded. Subdirectories of allowed directories are also permitted. + /// + /// + /// 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. + /// + public IEnumerable AllowedUploadDirectories + { + get => this._allowedUploadDirectories; + set => this._allowedUploadDirectories = value is null ? [] : new HashSet(value, StringComparer.OrdinalIgnoreCase); + } + /// /// Get the contents of a file stored in a cloud drive. /// @@ -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); } /// @@ -100,4 +136,59 @@ public async Task 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; + + /// + /// 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. + /// + 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 } diff --git a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs index ee15a1a92725..a6e34f14dbe2 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs +++ b/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 connectorMock = new(); connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) .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 connectorMock = new(); + CloudDrivePlugin target = new(connectorMock.Object); + + // Act & Assert — default config denies all paths + await Assert.ThrowsAsync(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 connectorMock = new(); + CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] }; + + // Act & Assert — traversal path is canonicalized and rejected + await Assert.ThrowsAsync(async () => + await target.UploadFileAsync(traversalPath, "/remote.txt")); + } + + [Fact] + public async Task ItDeniesUncPathsAsync() + { + // Arrange + Mock 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(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 connectorMock = new(); + CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] }; + + // Act & Assert + await Assert.ThrowsAsync(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 connectorMock = new(); + connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .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 connectorMock = new(); + connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .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 connectorMock = new(); + CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] }; + + // Act & Assert — expanded path is outside allowed directory + await Assert.ThrowsAsync(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 connectorMock = new(); + connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .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(async () => + await target.UploadFileAsync(filePath, "/remote.txt")); + } + } }