Skip to content

Commit 082700e

Browse files
SergeyMenshykhCopilot
SergeyMenshykh
and
Copilot
authored
.Net: Align plugin path validation (#14476)
Align path validation behavior across plugins and add regression coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent c7ba1af commit 082700e

4 files changed

Lines changed: 157 additions & 11 deletions

File tree

‎dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -161,15 +161,29 @@ private static string CanonicalizePath(string path)
161161
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
162162
}
163163

164-
return PathUtilities.GetSafeFullPath(expanded);
165-
}
164+
// Resolve the full path first (a pure string operation that does not touch the
165+
// filesystem). A relative path can still resolve to a UNC path here, for example
166+
// when the current directory is a UNC share, so re-check before GetSafeFullPath
167+
// probes the filesystem while resolving symbolic links.
168+
var fullPath = Path.GetFullPath(expanded);
169+
if (IsUncOrExtendedPath(fullPath))
170+
{
171+
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
172+
}
166173

167-
private static bool IsUncOrExtendedPath(string path)
168-
{
169-
return path.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase) ||
170-
path.StartsWith("//", StringComparison.OrdinalIgnoreCase);
174+
return PathUtilities.GetSafeFullPath(fullPath);
171175
}
172176

177+
/// <summary>
178+
/// Returns true if the path starts with any combination of two directory separators
179+
/// (e.g. <c>\\</c>, <c>//</c>, <c>/\</c>, <c>\/</c>), all of which resolve to UNC or
180+
/// extended-length/device paths on Windows.
181+
/// </summary>
182+
private static bool IsUncOrExtendedPath(string path) =>
183+
path.Length >= 2 &&
184+
(path[0] is '/' or '\\') &&
185+
(path[1] is '/' or '\\');
186+
173187
/// <summary>
174188
/// Checks whether a canonicalized file path falls within one of the allowed directories.
175189
/// Subdirectories of allowed directories are also permitted.

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

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -298,8 +298,7 @@ private static string CanonicalizePath(string path)
298298
{
299299
Ensure.NotNullOrWhitespace(path, nameof(path));
300300

301-
if (path.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase) ||
302-
path.StartsWith("//", StringComparison.OrdinalIgnoreCase))
301+
if (IsUncOrExtendedPath(path))
303302
{
304303
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
305304
}
@@ -310,15 +309,32 @@ private static string CanonicalizePath(string path)
310309

311310
// Re-check after expansion: an env var could have expanded to a UNC
312311
// or extended-path prefix (e.g., %NETSHARE% → \\server\share).
313-
if (expanded.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase) ||
314-
expanded.StartsWith("//", StringComparison.OrdinalIgnoreCase))
312+
if (IsUncOrExtendedPath(expanded))
315313
{
316314
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
317315
}
318316

319-
return Path.GetFullPath(expanded);
317+
// Re-check after canonicalization: a relative path can still resolve to a UNC path,
318+
// for example when the current directory is a UNC share.
319+
var fullPath = Path.GetFullPath(expanded);
320+
if (IsUncOrExtendedPath(fullPath))
321+
{
322+
throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path));
323+
}
324+
325+
return fullPath;
320326
}
321327

328+
/// <summary>
329+
/// Returns true if the path starts with any combination of two directory separators
330+
/// (e.g. <c>\\</c>, <c>//</c>, <c>/\</c>, <c>\/</c>), all of which resolve to UNC or
331+
/// extended-length/device paths on Windows.
332+
/// </summary>
333+
private static bool IsUncOrExtendedPath(string path) =>
334+
path.Length >= 2 &&
335+
(path[0] is '/' or '\\') &&
336+
(path[1] is '/' or '\\');
337+
322338
/// <summary>
323339
/// Checks whether a canonicalized file path falls within one of the allowed upload directories.
324340
/// Subdirectories of allowed directories are also permitted.

‎dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,78 @@ public async Task ItDeniesUncPathsAsync()
192192
await Assert.ThrowsAnyAsync<Exception>(async () => await target.AppendTextAsync("text", "//UNC/server/folder/file.docx"));
193193
}
194194

195+
[Theory]
196+
[InlineData("\\\\UNC\\server\\folder\\file.docx")]
197+
[InlineData("//UNC/server/folder/file.docx")]
198+
[InlineData("/\\UNC\\server\\folder\\file.docx")]
199+
[InlineData("\\/UNC/server/folder/file.docx")]
200+
[InlineData("\\\\?\\UNC\\server\\folder\\file.docx")]
201+
[InlineData("\\\\.\\UNC\\server\\folder\\file.docx")]
202+
public async Task ItRejectsUncOrExtendedPathsOnReadAsync(string path)
203+
{
204+
// Arrange
205+
var fileSystemConnectorMock = new Mock<IFileSystemConnector>(MockBehavior.Strict);
206+
var documentConnectorMock = new Mock<IDocumentConnector>(MockBehavior.Strict);
207+
var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
208+
{
209+
AllowedDirectories = [Path.GetTempPath()]
210+
};
211+
212+
// Act & Assert — rejected before any filesystem access
213+
await Assert.ThrowsAsync<ArgumentException>(async () => await target.ReadTextAsync(path));
214+
}
215+
216+
[Theory]
217+
[InlineData("\\\\UNC\\server\\folder\\file.docx")]
218+
[InlineData("//UNC/server/folder/file.docx")]
219+
[InlineData("/\\UNC\\server\\folder\\file.docx")]
220+
[InlineData("\\/UNC/server/folder/file.docx")]
221+
[InlineData("\\\\?\\UNC\\server\\folder\\file.docx")]
222+
[InlineData("\\\\.\\UNC\\server\\folder\\file.docx")]
223+
public async Task ItRejectsUncOrExtendedPathsOnAppendAsync(string path)
224+
{
225+
// Arrange
226+
var fileSystemConnectorMock = new Mock<IFileSystemConnector>(MockBehavior.Strict);
227+
var documentConnectorMock = new Mock<IDocumentConnector>(MockBehavior.Strict);
228+
var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
229+
{
230+
AllowedDirectories = [Path.GetTempPath()]
231+
};
232+
233+
// Act & Assert — rejected before any filesystem access
234+
await Assert.ThrowsAsync<ArgumentException>(async () => await target.AppendTextAsync("text", path));
235+
}
236+
237+
[Theory]
238+
[InlineData("/\\evil-server\\share")]
239+
[InlineData("\\/evil-server/share")]
240+
public async Task ItDeniesMixedSeparatorUncPathsIntroducedViaEnvVarExpansionAsync(string envVarValue)
241+
{
242+
// Arrange — env var that expands to a mixed-separator UNC path
243+
var envVarName = "SK_TEST_MIXUNC_" + Guid.NewGuid().ToString("N")[..8];
244+
245+
try
246+
{
247+
Environment.SetEnvironmentVariable(envVarName, envVarValue);
248+
var maliciousPath = $"%{envVarName}%{Path.DirectorySeparatorChar}secret.docx";
249+
250+
var fileSystemConnectorMock = new Mock<IFileSystemConnector>(MockBehavior.Strict);
251+
var documentConnectorMock = new Mock<IDocumentConnector>(MockBehavior.Strict);
252+
var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
253+
{
254+
AllowedDirectories = [Path.GetTempPath()]
255+
};
256+
257+
// Act & Assert
258+
await Assert.ThrowsAsync<ArgumentException>(async () => await target.ReadTextAsync(maliciousPath));
259+
await Assert.ThrowsAsync<ArgumentException>(async () => await target.AppendTextAsync("text", maliciousPath));
260+
}
261+
finally
262+
{
263+
Environment.SetEnvironmentVariable(envVarName, null);
264+
}
265+
}
266+
195267
[Fact]
196268
public async Task ItDeniesDisallowedFoldersAsync()
197269
{

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,50 @@ await Assert.ThrowsAsync<ArgumentException>(async () =>
361361
await target.UploadFileAsync("//server/share/file.txt", "/remote.txt"));
362362
}
363363

364+
[Theory]
365+
[InlineData("\\\\UNC\\server\\folder\\file.txt")]
366+
[InlineData("//UNC/server/folder/file.txt")]
367+
[InlineData("/\\UNC\\server\\folder\\file.txt")]
368+
[InlineData("\\/UNC/server/folder/file.txt")]
369+
[InlineData("\\\\?\\UNC\\server\\folder\\file.txt")]
370+
[InlineData("\\\\.\\UNC\\server\\folder\\file.txt")]
371+
public async Task ItRejectsUncOrExtendedPathsOnUploadAsync(string path)
372+
{
373+
// Arrange
374+
Mock<ICloudDriveConnector> connectorMock = new(MockBehavior.Strict);
375+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [Path.GetTempPath()], AllowedUploadDestinationPaths = ["/"] };
376+
377+
// Act & Assert — rejected before the connector is invoked
378+
await Assert.ThrowsAsync<ArgumentException>(async () =>
379+
await target.UploadFileAsync(path, "/remote.txt"));
380+
}
381+
382+
[Theory]
383+
[InlineData("/\\server\\share")]
384+
[InlineData("\\/server/share")]
385+
public async Task ItDeniesEnvVarExpansionToMixedSeparatorUncPathAsync(string envVarValue)
386+
{
387+
// Arrange — env var that expands to a mixed-separator UNC path should be rejected
388+
var envVarName = "SK_TEST_MIXUNC_" + Guid.NewGuid().ToString("N")[..8];
389+
390+
try
391+
{
392+
Environment.SetEnvironmentVariable(envVarName, envVarValue);
393+
var maliciousPath = $"%{envVarName}%{Path.DirectorySeparatorChar}secret.txt";
394+
395+
Mock<ICloudDriveConnector> connectorMock = new(MockBehavior.Strict);
396+
CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [Path.GetTempPath()], AllowedUploadDestinationPaths = ["/"] };
397+
398+
// Act & Assert
399+
await Assert.ThrowsAsync<ArgumentException>(async () =>
400+
await target.UploadFileAsync(maliciousPath, "/remote.txt"));
401+
}
402+
finally
403+
{
404+
Environment.SetEnvironmentVariable(envVarName, null);
405+
}
406+
}
407+
364408
[Fact]
365409
public async Task ItDeniesEnvVarExpansionToForwardSlashUncPathAsync()
366410
{

0 commit comments

Comments
 (0)