Skip to content

Commit 3dd139b

Browse files
.Net: Harden CloudDrivePlugin defaults and add path validation (#13958)
### Description - Improve default settings in CloudDrivePlugin - Add path validation for share-link operations - Improve path canonicalization - Add regression tests ### Contribution Checklist - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) - [x] The existing tests pass - [x] New tests added --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 446c2ef commit 3dd139b

4 files changed

Lines changed: 544 additions & 36 deletions

File tree

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

Lines changed: 184 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,25 @@ namespace Microsoft.SemanticKernel.Plugins.MsGraph;
1818
/// </summary>
1919
/// <remarks>
2020
/// <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.
21+
/// This plugin is secure by default. <see cref="AllowedUploadDirectories"/>, <see cref="AllowedUploadDestinationPaths"/>,
22+
/// <see cref="AllowedReadPaths"/>, and <see cref="AllowedSharePaths"/> must be explicitly configured
23+
/// before file upload, read, or share-link operations are permitted.
24+
/// By default, all paths are denied.
2325
/// </para>
2426
/// <para>
2527
/// When exposing this plugin to an LLM via auto function calling, ensure that
26-
/// <see cref="AllowedUploadDirectories"/> is restricted to trusted values only.
28+
/// <see cref="AllowedUploadDirectories"/>, <see cref="AllowedUploadDestinationPaths"/>, <see cref="AllowedReadPaths"/>,
29+
/// and <see cref="AllowedSharePaths"/> are restricted to trusted values only.
2730
/// </para>
2831
/// </remarks>
2932
public sealed class CloudDrivePlugin
3033
{
3134
private readonly ICloudDriveConnector _connector;
3235
private readonly ILogger _logger;
3336
private HashSet<string> _allowedUploadDirectories = [];
37+
private HashSet<string> _allowedSharePaths = [];
38+
private HashSet<string> _allowedReadPaths = [];
39+
private HashSet<string> _allowedUploadDestinationPaths = [];
3440

3541
/// <summary>
3642
/// Initializes a new instance of the <see cref="CloudDrivePlugin"/> class.
@@ -59,6 +65,57 @@ public IEnumerable<string> AllowedUploadDirectories
5965
set => this._allowedUploadDirectories = value is null ? [] : new HashSet<string>(value, StringComparer.OrdinalIgnoreCase);
6066
}
6167

68+
/// <summary>
69+
/// List of allowed remote directory prefixes for which sharing links may be created.
70+
/// A file is permitted if its parent directory starts with (or equals) any entry in this list.
71+
/// Subdirectories of allowed paths are also permitted.
72+
/// </summary>
73+
/// <remarks>
74+
/// Defaults to an empty collection (no paths allowed). Must be explicitly populated
75+
/// with trusted remote directory paths before any share-link operations will succeed.
76+
/// Paths are normalized with forward slashes and dot-segments are collapsed before comparison.
77+
/// Matching is case-insensitive (OneDrive paths are case-insensitive).
78+
/// </remarks>
79+
public IEnumerable<string> AllowedSharePaths
80+
{
81+
get => this._allowedSharePaths;
82+
set => this._allowedSharePaths = value is null ? [] : new HashSet<string>(value, StringComparer.OrdinalIgnoreCase);
83+
}
84+
85+
/// <summary>
86+
/// List of allowed remote directory prefixes from which file contents may be read.
87+
/// A file is permitted if its parent directory starts with (or equals) any entry in this list.
88+
/// Subdirectories of allowed paths are also permitted.
89+
/// </summary>
90+
/// <remarks>
91+
/// Defaults to an empty collection (no paths allowed). Must be explicitly populated
92+
/// with trusted remote directory paths before any read operations will succeed.
93+
/// Paths are normalized with forward slashes and dot-segments are collapsed before comparison.
94+
/// Matching is case-insensitive (OneDrive paths are case-insensitive).
95+
/// </remarks>
96+
public IEnumerable<string> AllowedReadPaths
97+
{
98+
get => this._allowedReadPaths;
99+
set => this._allowedReadPaths = value is null ? [] : new HashSet<string>(value, StringComparer.OrdinalIgnoreCase);
100+
}
101+
102+
/// <summary>
103+
/// List of allowed remote directory prefixes to which files may be uploaded.
104+
/// A destination is permitted if its parent directory starts with (or equals) any entry in this list.
105+
/// Subdirectories of allowed paths are also permitted.
106+
/// </summary>
107+
/// <remarks>
108+
/// Defaults to an empty collection (no paths allowed). Must be explicitly populated
109+
/// with trusted remote directory paths before any upload-destination operations will succeed.
110+
/// Paths are normalized with forward slashes and dot-segments are collapsed before comparison.
111+
/// Matching is case-insensitive (OneDrive paths are case-insensitive).
112+
/// </remarks>
113+
public IEnumerable<string> AllowedUploadDestinationPaths
114+
{
115+
get => this._allowedUploadDestinationPaths;
116+
set => this._allowedUploadDestinationPaths = value is null ? [] : new HashSet<string>(value, StringComparer.OrdinalIgnoreCase);
117+
}
118+
62119
/// <summary>
63120
/// Get the contents of a file stored in a cloud drive.
64121
/// </summary>
@@ -71,6 +128,14 @@ public IEnumerable<string> AllowedUploadDirectories
71128
CancellationToken cancellationToken = default)
72129
{
73130
this._logger.LogDebug("Getting file content for '{0}'", filePath);
131+
132+
Ensure.NotNullOrWhitespace(filePath, nameof(filePath));
133+
134+
if (!this.IsAllowedRemotePath(filePath, this._allowedReadPaths))
135+
{
136+
throw new InvalidOperationException("Reading from the provided path is not allowed. Configure 'AllowedReadPaths' with trusted remote paths to enable reading.");
137+
}
138+
74139
Stream? fileContentStream = await this._connector.GetFileContentStreamAsync(filePath, cancellationToken).ConfigureAwait(false);
75140

76141
if (fileContentStream is null)
@@ -104,9 +169,14 @@ public async Task UploadFileAsync(
104169
throw new ArgumentException("Variable was null or whitespace", nameof(destinationPath));
105170
}
106171

172+
if (!this.IsAllowedRemotePath(destinationPath, this._allowedUploadDestinationPaths))
173+
{
174+
throw new InvalidOperationException("Uploading to the provided remote destination is not allowed. Configure 'AllowedUploadDestinationPaths' with trusted remote paths to enable uploads.");
175+
}
176+
107177
Ensure.NotNullOrWhitespace(filePath, nameof(filePath));
108178

109-
var canonicalPath = Path.GetFullPath(Environment.ExpandEnvironmentVariables(filePath));
179+
var canonicalPath = CanonicalizePath(filePath);
110180

111181
if (!this.IsUploadPathAllowed(canonicalPath))
112182
{
@@ -131,8 +201,15 @@ public async Task<string> CreateLinkAsync(
131201
CancellationToken cancellationToken = default)
132202
{
133203
this._logger.LogDebug("Creating link for '{0}'", filePath);
134-
const string Type = "view"; // TODO expose this as an SK variable
135-
const string Scope = "anonymous"; // TODO expose this as an SK variable
204+
const string Type = "view";
205+
const string Scope = "organization";
206+
207+
Ensure.NotNullOrWhitespace(filePath, nameof(filePath));
208+
209+
if (!this.IsAllowedRemotePath(filePath, this._allowedSharePaths))
210+
{
211+
throw new InvalidOperationException("Creating a share link for the provided path is not allowed. Configure 'AllowedSharePaths' with trusted remote paths to enable sharing.");
212+
}
136213

137214
return await this._connector.CreateShareLinkAsync(filePath, Type, Scope, cancellationToken).ConfigureAwait(false);
138215
}
@@ -145,33 +222,123 @@ public async Task<string> CreateLinkAsync(
145222
: StringComparison.Ordinal;
146223

147224
/// <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.
225+
/// Checks whether the provided remote path falls within one of the allowed remote directory prefixes.
226+
/// Paths are normalized with forward slashes, dot-segments are collapsed,
227+
/// and compared case-insensitively (OneDrive paths are case-insensitive).
228+
/// Subdirectories of allowed paths are permitted.
151229
/// </summary>
152-
private bool IsUploadPathAllowed(string path)
230+
private bool IsAllowedRemotePath(string path, HashSet<string> allowedPaths)
153231
{
154232
Ensure.NotNullOrWhitespace(path, nameof(path));
155233

156-
if (path.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase))
234+
if (allowedPaths.Count == 0)
235+
{
236+
return false;
237+
}
238+
239+
// Normalize to forward slashes and collapse dot-segments to prevent traversal bypass.
240+
var normalizedPath = NormalizeRemotePath(path);
241+
242+
foreach (var allowedPath in allowedPaths)
243+
{
244+
var normalizedAllowed = NormalizeRemotePath(allowedPath);
245+
if (!normalizedAllowed.EndsWith("/", StringComparison.Ordinal))
246+
{
247+
normalizedAllowed += "/";
248+
}
249+
250+
var normalizedDir = normalizedPath;
251+
int lastSlash = normalizedDir.LastIndexOf('/');
252+
if (lastSlash >= 0)
253+
{
254+
normalizedDir = normalizedDir.Substring(0, lastSlash);
255+
}
256+
257+
if ((normalizedDir + "/").StartsWith(normalizedAllowed, StringComparison.OrdinalIgnoreCase)
258+
|| (normalizedDir + "/").Equals(normalizedAllowed, StringComparison.OrdinalIgnoreCase))
259+
{
260+
return true;
261+
}
262+
}
263+
264+
return false;
265+
}
266+
267+
/// <summary>
268+
/// Normalizes a remote path by replacing backslashes with forward slashes
269+
/// and collapsing "." and ".." segments to prevent traversal bypass.
270+
/// </summary>
271+
private static string NormalizeRemotePath(string path)
272+
{
273+
var normalizedPath = path.Replace('\\', '/');
274+
275+
// Collapse ".." and "." segments to prevent traversal bypass.
276+
var segments = normalizedPath.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries);
277+
var stack = new List<string>();
278+
foreach (var segment in segments)
279+
{
280+
if (segment == ".." && stack.Count > 0)
281+
{
282+
stack.RemoveAt(stack.Count - 1);
283+
}
284+
else if (segment != "." && segment != "..")
285+
{
286+
stack.Add(segment);
287+
}
288+
}
289+
290+
return "/" + string.Join("/", stack);
291+
}
292+
293+
/// <summary>
294+
/// Expands environment variables and resolves the path to its canonical form.
295+
/// This must be called before validation to prevent validate/use mismatches.
296+
/// </summary>
297+
private static string CanonicalizePath(string path)
298+
{
299+
Ensure.NotNullOrWhitespace(path, nameof(path));
300+
301+
if (path.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase) ||
302+
path.StartsWith("//", StringComparison.OrdinalIgnoreCase))
157303
{
158304
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
159305
}
160306

161-
string? directoryPath = Path.GetDirectoryName(path);
307+
// Expand environment variables first, then canonicalize — so that
308+
// validation and I/O operate on the same resolved path.
309+
var expanded = Environment.ExpandEnvironmentVariables(path);
310+
311+
// Re-check after expansion: an env var could have expanded to a UNC
312+
// or extended-path prefix (e.g., %NETSHARE% → \\server\share).
313+
if (expanded.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase) ||
314+
expanded.StartsWith("//", StringComparison.OrdinalIgnoreCase))
315+
{
316+
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
317+
}
318+
319+
return Path.GetFullPath(expanded);
320+
}
321+
322+
/// <summary>
323+
/// Checks whether a canonicalized file path falls within one of the allowed upload directories.
324+
/// Subdirectories of allowed directories are also permitted.
325+
/// </summary>
326+
private bool IsUploadPathAllowed(string canonicalPath)
327+
{
328+
Ensure.NotNullOrWhitespace(canonicalPath, nameof(canonicalPath));
329+
330+
string? directoryPath = Path.GetDirectoryName(canonicalPath);
162331

163332
if (string.IsNullOrEmpty(directoryPath))
164333
{
165-
throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(path));
334+
throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(canonicalPath));
166335
}
167336

168337
if (this._allowedUploadDirectories.Count == 0)
169338
{
170339
return false;
171340
}
172341

173-
var canonicalDir = Path.GetFullPath(directoryPath);
174-
175342
foreach (var allowedDirectory in this._allowedUploadDirectories)
176343
{
177344
var canonicalAllowed = Path.GetFullPath(allowedDirectory);
@@ -181,8 +348,8 @@ private bool IsUploadPathAllowed(string path)
181348
canonicalAllowed += separator;
182349
}
183350

184-
if (canonicalDir.StartsWith(canonicalAllowed, s_pathComparison)
185-
|| (canonicalDir + separator).Equals(canonicalAllowed, s_pathComparison))
351+
if (directoryPath.StartsWith(canonicalAllowed, s_pathComparison)
352+
|| (directoryPath + separator).Equals(canonicalAllowed, s_pathComparison))
186353
{
187354
return true;
188355
}

dotnet/src/Plugins/Plugins.MsGraph/Connectors/OneDriveConnector.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ public async Task UploadSmallFileAsync(string filePath, string destinationPath,
114114
}
115115

116116
/// <inheritdoc/>
117-
public async Task<string> CreateShareLinkAsync(string filePath, string type = "view", string scope = "anonymous",
117+
public async Task<string> CreateShareLinkAsync(string filePath, string type = "view", string scope = "organization",
118118
CancellationToken cancellationToken = default)
119119
{
120120
Ensure.NotNullOrWhitespace(filePath, nameof(filePath));

dotnet/src/Plugins/Plugins.MsGraph/ICloudDriveConnector.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ public interface ICloudDriveConnector
1919
/// <param name="scope">Scope of the link.</param>
2020
/// <param name="cancellationToken">The <see cref="CancellationToken"/> to monitor for cancellation requests. The default is <see cref="CancellationToken.None"/>.</param>
2121
/// <returns>Shareable link.</returns>
22-
Task<string> CreateShareLinkAsync(string filePath, string type = "view", string scope = "anonymous", CancellationToken cancellationToken = default);
22+
Task<string> CreateShareLinkAsync(string filePath, string type = "view", string scope = "organization", CancellationToken cancellationToken = default);
2323

2424
/// <summary>
2525
/// Get the content of a file.

0 commit comments

Comments
 (0)