Skip to content

Commit 2a719ca

Browse files
.Net: Add deny-by-default AllowedUploadDirectories to CloudDrivePlugin (#13953)
### Description Add deny-by-default `AllowedUploadDirectories` to `CloudDrivePlugin`. - Added `AllowedUploadDirectories` property (defaults to empty = deny-all) - `UploadFileAsync` now validates the local file path against the allowlist before uploading - Paths are canonicalized with environment variable expansion, UNC rejection, and directory traversal protection - Added unit tests for the new behavior --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 006a5d9 commit 2a719ca

2 files changed

Lines changed: 257 additions & 4 deletions

File tree

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

Lines changed: 93 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
// Copyright (c) Microsoft. All rights reserved.
22

33
using System;
4+
using System.Collections.Generic;
45
using System.ComponentModel;
56
using System.IO;
7+
using System.Runtime.InteropServices;
68
using System.Threading;
79
using System.Threading.Tasks;
810
using Microsoft.Extensions.Logging;
@@ -14,10 +16,21 @@ namespace Microsoft.SemanticKernel.Plugins.MsGraph;
1416
/// <summary>
1517
/// Cloud drive plugin (e.g. OneDrive).
1618
/// </summary>
19+
/// <remarks>
20+
/// <para>
21+
/// This plugin is secure by default. <see cref="AllowedUploadDirectories"/> must be explicitly configured
22+
/// before any file upload operations are permitted. By default, all local file paths are denied.
23+
/// </para>
24+
/// <para>
25+
/// When exposing this plugin to an LLM via auto function calling, ensure that
26+
/// <see cref="AllowedUploadDirectories"/> is restricted to trusted values only.
27+
/// </para>
28+
/// </remarks>
1729
public sealed class CloudDrivePlugin
1830
{
1931
private readonly ICloudDriveConnector _connector;
2032
private readonly ILogger _logger;
33+
private HashSet<string> _allowedUploadDirectories = [];
2134

2235
/// <summary>
2336
/// Initializes a new instance of the <see cref="CloudDrivePlugin"/> class.
@@ -32,6 +45,20 @@ public CloudDrivePlugin(ICloudDriveConnector connector, ILoggerFactory? loggerFa
3245
this._logger = loggerFactory?.CreateLogger(typeof(CloudDrivePlugin)) ?? NullLogger.Instance;
3346
}
3447

48+
/// <summary>
49+
/// List of allowed local directories from which files may be uploaded. Subdirectories of allowed directories are also permitted.
50+
/// </summary>
51+
/// <remarks>
52+
/// Defaults to an empty collection (no directories allowed). Must be explicitly populated
53+
/// with trusted directory paths before any file upload operations will succeed.
54+
/// Paths are canonicalized before validation to prevent directory traversal.
55+
/// </remarks>
56+
public IEnumerable<string> AllowedUploadDirectories
57+
{
58+
get => this._allowedUploadDirectories;
59+
set => this._allowedUploadDirectories = value is null ? [] : new HashSet<string>(value, StringComparer.OrdinalIgnoreCase);
60+
}
61+
3562
/// <summary>
3663
/// Get the contents of a file stored in a cloud drive.
3764
/// </summary>
@@ -77,10 +104,19 @@ public async Task UploadFileAsync(
77104
throw new ArgumentException("Variable was null or whitespace", nameof(destinationPath));
78105
}
79106

80-
this._logger.LogDebug("Uploading file '{0}'", filePath);
107+
Ensure.NotNullOrWhitespace(filePath, nameof(filePath));
108+
109+
var canonicalPath = Path.GetFullPath(Environment.ExpandEnvironmentVariables(filePath));
110+
111+
if (!this.IsUploadPathAllowed(canonicalPath))
112+
{
113+
throw new InvalidOperationException("Uploading from the provided location is not allowed. Configure 'AllowedUploadDirectories' with trusted directory paths to enable uploads.");
114+
}
115+
116+
this._logger.LogDebug("Uploading file '{0}'", canonicalPath);
81117

82118
// TODO Add support for large file uploads (i.e. upload sessions)
83-
await this._connector.UploadSmallFileAsync(filePath, destinationPath, cancellationToken).ConfigureAwait(false);
119+
await this._connector.UploadSmallFileAsync(canonicalPath, destinationPath, cancellationToken).ConfigureAwait(false);
84120
}
85121

86122
/// <summary>
@@ -100,4 +136,59 @@ public async Task<string> CreateLinkAsync(
100136

101137
return await this._connector.CreateShareLinkAsync(filePath, Type, Scope, cancellationToken).ConfigureAwait(false);
102138
}
139+
140+
#region private
141+
// Use case-insensitive comparison on Windows (case-insensitive FS), case-sensitive on Linux/macOS.
142+
private static readonly StringComparison s_pathComparison =
143+
RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
144+
? StringComparison.OrdinalIgnoreCase
145+
: StringComparison.Ordinal;
146+
147+
/// <summary>
148+
/// If a list of allowed upload directories has been provided, the directory of the provided filePath is checked
149+
/// to verify it is in the allowed directory list. Paths are canonicalized before comparison.
150+
/// Subdirectories of allowed directories are also permitted.
151+
/// </summary>
152+
private bool IsUploadPathAllowed(string path)
153+
{
154+
Ensure.NotNullOrWhitespace(path, nameof(path));
155+
156+
if (path.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase))
157+
{
158+
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
159+
}
160+
161+
string? directoryPath = Path.GetDirectoryName(path);
162+
163+
if (string.IsNullOrEmpty(directoryPath))
164+
{
165+
throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(path));
166+
}
167+
168+
if (this._allowedUploadDirectories.Count == 0)
169+
{
170+
return false;
171+
}
172+
173+
var canonicalDir = Path.GetFullPath(directoryPath);
174+
175+
foreach (var allowedDirectory in this._allowedUploadDirectories)
176+
{
177+
var canonicalAllowed = Path.GetFullPath(allowedDirectory);
178+
var separator = Path.DirectorySeparatorChar.ToString();
179+
if (!canonicalAllowed.EndsWith(separator, s_pathComparison))
180+
{
181+
canonicalAllowed += separator;
182+
}
183+
184+
if (canonicalDir.StartsWith(canonicalAllowed, s_pathComparison)
185+
|| (canonicalDir + separator).Equals(canonicalAllowed, s_pathComparison))
186+
{
187+
return true;
188+
}
189+
}
190+
191+
return false;
192+
}
193+
#endregion
103194
}

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

Lines changed: 164 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
using System;
44
using System.IO;
5+
using System.Runtime.InteropServices;
56
using System.Text;
67
using System.Threading;
78
using System.Threading.Tasks;
@@ -17,13 +18,14 @@ public class CloudDrivePluginTests
1718
public async Task UploadSmallFileAsyncSucceedsAsync()
1819
{
1920
// Arrange
20-
string anyFilePath = Guid.NewGuid().ToString();
21+
string allowedDir = Path.GetTempPath();
22+
string anyFilePath = Path.Combine(allowedDir, Guid.NewGuid().ToString());
2123

2224
Mock<ICloudDriveConnector> connectorMock = new();
2325
connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
2426
.Returns(Task.CompletedTask);
2527

26-
CloudDrivePlugin target = new(connectorMock.Object);
28+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
2729

2830
// Act
2931
await target.UploadFileAsync(anyFilePath, Guid.NewGuid().ToString());
@@ -74,4 +76,164 @@ public async Task GetFileContentAsyncSucceedsAsync()
7476
Assert.Equal(expectedContent, actual);
7577
connectorMock.VerifyAll();
7678
}
79+
80+
[Fact]
81+
public async Task ItDeniesAllPathsByDefaultAsync()
82+
{
83+
// Arrange
84+
string filePath = Path.Combine(Path.GetTempPath(), "somefile.txt");
85+
86+
Mock<ICloudDriveConnector> connectorMock = new();
87+
CloudDrivePlugin target = new(connectorMock.Object);
88+
89+
// Act & Assert — default config denies all paths
90+
await Assert.ThrowsAsync<InvalidOperationException>(async () =>
91+
await target.UploadFileAsync(filePath, "/remote.txt"));
92+
}
93+
94+
[Fact]
95+
public async Task ItDeniesPathTraversalAsync()
96+
{
97+
// Arrange
98+
var allowedDir = Path.Combine(Path.GetTempPath(), "allowed-folder");
99+
var traversalPath = Path.Combine(allowedDir, "..", "outside-folder", "secret.txt");
100+
101+
Mock<ICloudDriveConnector> connectorMock = new();
102+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
103+
104+
// Act & Assert — traversal path is canonicalized and rejected
105+
await Assert.ThrowsAsync<InvalidOperationException>(async () =>
106+
await target.UploadFileAsync(traversalPath, "/remote.txt"));
107+
}
108+
109+
[Fact]
110+
public async Task ItDeniesUncPathsAsync()
111+
{
112+
// Arrange
113+
Mock<ICloudDriveConnector> connectorMock = new();
114+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [Path.GetTempPath()] };
115+
116+
// Act & Assert — UNC paths are rejected (ArgumentException on Windows, InvalidOperationException on Linux
117+
// where the path is canonicalized differently and fails the allowlist check instead)
118+
await Assert.ThrowsAnyAsync<Exception>(async () =>
119+
await target.UploadFileAsync("\\\\UNC\\server\\folder\\file.txt", "/remote.txt"));
120+
}
121+
122+
[Fact]
123+
public async Task ItDeniesDisallowedDirectoriesAsync()
124+
{
125+
// Arrange
126+
var allowedDir = Path.Combine(Path.GetTempPath(), "allowed");
127+
var disallowedPath = Path.Combine(Path.GetTempPath(), "disallowed", "file.txt");
128+
129+
Mock<ICloudDriveConnector> connectorMock = new();
130+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
131+
132+
// Act & Assert
133+
await Assert.ThrowsAsync<InvalidOperationException>(async () =>
134+
await target.UploadFileAsync(disallowedPath, "/remote.txt"));
135+
}
136+
137+
[Fact]
138+
public async Task ItAllowsSubdirectoriesOfAllowedDirectoriesAsync()
139+
{
140+
// Arrange
141+
var allowedDir = Path.GetTempPath();
142+
var subDirPath = Path.Combine(allowedDir, "subdir", "nested", "file.txt");
143+
144+
Mock<ICloudDriveConnector> connectorMock = new();
145+
connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
146+
.Returns(Task.CompletedTask);
147+
148+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
149+
150+
// Act — subdirectory of allowed folder should succeed
151+
await target.UploadFileAsync(subDirPath, "/remote.txt");
152+
153+
// Assert
154+
connectorMock.VerifyAll();
155+
}
156+
157+
[Fact]
158+
public async Task ItExpandsEnvironmentVariablesAndValidatesAsync()
159+
{
160+
// Arrange — set a dedicated test env var to avoid platform-specific assumptions
161+
var tempDir = Path.GetTempPath().TrimEnd(Path.DirectorySeparatorChar);
162+
var envVarName = "SK_TEST_UPLOAD_DIR";
163+
var originalValue = Environment.GetEnvironmentVariable(envVarName);
164+
try
165+
{
166+
Environment.SetEnvironmentVariable(envVarName, tempDir);
167+
var envVarPath = Path.Combine($"%{envVarName}%", "testfile.txt");
168+
169+
Mock<ICloudDriveConnector> connectorMock = new();
170+
connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
171+
.Returns(Task.CompletedTask);
172+
173+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [tempDir] };
174+
175+
// Act — env var should be expanded and path should be allowed
176+
await target.UploadFileAsync(envVarPath, "/remote.txt");
177+
178+
// Assert
179+
connectorMock.VerifyAll();
180+
}
181+
finally
182+
{
183+
Environment.SetEnvironmentVariable(envVarName, originalValue);
184+
}
185+
}
186+
187+
[Fact]
188+
public async Task ItDeniesExpandedEnvironmentVariablePathsOutsideAllowedAsync()
189+
{
190+
// Arrange — set a dedicated test env var; allow a subdirectory but env var expands outside it
191+
var tempDir = Path.GetTempPath().TrimEnd(Path.DirectorySeparatorChar);
192+
var allowedDir = Path.Combine(tempDir, "specific-allowed");
193+
var envVarName = "SK_TEST_UPLOAD_DIR";
194+
var originalValue = Environment.GetEnvironmentVariable(envVarName);
195+
try
196+
{
197+
Environment.SetEnvironmentVariable(envVarName, tempDir);
198+
var envVarPath = Path.Combine($"%{envVarName}%", "outside-file.txt");
199+
200+
Mock<ICloudDriveConnector> connectorMock = new();
201+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
202+
203+
// Act & Assert — expanded path is outside allowed directory
204+
await Assert.ThrowsAsync<InvalidOperationException>(async () =>
205+
await target.UploadFileAsync(envVarPath, "/remote.txt"));
206+
}
207+
finally
208+
{
209+
Environment.SetEnvironmentVariable(envVarName, originalValue);
210+
}
211+
}
212+
213+
[Fact]
214+
public async Task ItRespectsPlatformCaseSensitivityAsync()
215+
{
216+
// Arrange — use differently-cased allowed dir vs file path
217+
var allowedDir = Path.Combine(Path.GetTempPath(), "AllowedFolder");
218+
var filePath = Path.Combine(Path.GetTempPath(), "allowedfolder", "file.txt");
219+
220+
Mock<ICloudDriveConnector> connectorMock = new();
221+
connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny<string>(), It.IsAny<string>(), It.IsAny<CancellationToken>()))
222+
.Returns(Task.CompletedTask);
223+
224+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir] };
225+
226+
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
227+
{
228+
// Windows: case-insensitive FS — differently-cased path should be allowed
229+
await target.UploadFileAsync(filePath, "/remote.txt");
230+
connectorMock.VerifyAll();
231+
}
232+
else
233+
{
234+
// Linux/macOS: case-sensitive FS — differently-cased path should be denied
235+
await Assert.ThrowsAsync<InvalidOperationException>(async () =>
236+
await target.UploadFileAsync(filePath, "/remote.txt"));
237+
}
238+
}
77239
}

0 commit comments

Comments
 (0)