From 5a60972366fb99ed13b4aeb181964af0edac5a28 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh Date: Wed, 6 May 2026 13:46:17 +0100 Subject: [PATCH 1/2] Fix DocumentPlugin path validation order Canonicalize file paths (expand environment variables and resolve to absolute form) before validating against AllowedDirectories, and remove redundant environment variable expansion from LocalFileSystemConnector. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Plugins.Document/DocumentPlugin.cs | 53 ++++++++++------ .../FileSystem/LocalFileSystemConnector.cs | 8 +-- .../Document/DocumentPluginTests.cs | 63 ++++++++++++++++++- 3 files changed, 98 insertions(+), 26 deletions(-) diff --git a/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs b/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs index f0f3a1fd37c8..0bf8aec91a15 100644 --- a/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs @@ -87,14 +87,16 @@ public async Task ReadTextAsync( [Description("Path to the file to read")] string filePath, CancellationToken cancellationToken = default) { - this._logger.LogDebug("Reading text from {0}", filePath); + var canonicalPath = CanonicalizePath(filePath); - if (!this.IsFilePathAllowed(filePath)) + this._logger.LogDebug("Reading text from {0}", canonicalPath); + + if (!this.IsFilePathAllowed(canonicalPath)) { throw new InvalidOperationException("Reading from the provided location is not allowed."); } - using var stream = await this._fileSystemConnector.GetFileContentStreamAsync(filePath, cancellationToken).ConfigureAwait(false); + using var stream = await this._fileSystemConnector.GetFileContentStreamAsync(canonicalPath, cancellationToken).ConfigureAwait(false); return this._documentConnector.ReadText(stream); } @@ -107,25 +109,27 @@ public async Task AppendTextAsync( [Description("Destination file path")] string filePath, CancellationToken cancellationToken = default) { - if (!this.IsFilePathAllowed(filePath)) + var canonicalPath = CanonicalizePath(filePath); + + if (!this.IsFilePathAllowed(canonicalPath)) { throw new InvalidOperationException("Writing to the provided location is not allowed."); } // If the document already exists, open it. If not, create it. - if (await this._fileSystemConnector.FileExistsAsync(filePath, cancellationToken).ConfigureAwait(false)) + if (await this._fileSystemConnector.FileExistsAsync(canonicalPath, cancellationToken).ConfigureAwait(false)) { - this._logger.LogDebug("Writing text to file {0}", filePath); - using Stream stream = await this._fileSystemConnector.GetWriteableFileStreamAsync(filePath, cancellationToken).ConfigureAwait(false); + this._logger.LogDebug("Writing text to file {0}", canonicalPath); + using Stream stream = await this._fileSystemConnector.GetWriteableFileStreamAsync(canonicalPath, cancellationToken).ConfigureAwait(false); this._documentConnector.AppendText(stream, text); } else { - this._logger.LogDebug("File does not exist. Creating file at {0}", filePath); - using Stream stream = await this._fileSystemConnector.CreateFileAsync(filePath, cancellationToken).ConfigureAwait(false); + this._logger.LogDebug("File does not exist. Creating file at {0}", canonicalPath); + using Stream stream = await this._fileSystemConnector.CreateFileAsync(canonicalPath, cancellationToken).ConfigureAwait(false); this._documentConnector.Initialize(stream); - this._logger.LogDebug("Writing text to {0}", filePath); + this._logger.LogDebug("Writing text to {0}", canonicalPath); this._documentConnector.AppendText(stream, text); } } @@ -134,11 +138,10 @@ public async Task AppendTextAsync( private HashSet? _allowedDirectories = []; /// - /// If a list of allowed 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. + /// Expands environment variables and resolves the path to its canonical form. + /// This must be called before validation to prevent validate/use mismatches. /// - private bool IsFilePathAllowed(string path) + private static string CanonicalizePath(string path) { Verify.NotNullOrWhiteSpace(path); @@ -147,11 +150,23 @@ private bool IsFilePathAllowed(string path) throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); } - string? directoryPath = Path.GetDirectoryName(path); + // Expand environment variables first, then canonicalize — so that + // validation and I/O operate on the same resolved path. + var expanded = Environment.ExpandEnvironmentVariables(path); + return Path.GetFullPath(expanded); + } + + /// + /// Checks whether a canonicalized file path falls within one of the allowed directories. + /// Subdirectories of allowed directories are also permitted. + /// + private bool IsFilePathAllowed(string canonicalPath) + { + string? directoryPath = Path.GetDirectoryName(canonicalPath); if (string.IsNullOrEmpty(directoryPath)) { - throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(path)); + throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(canonicalPath)); } if (this._allowedDirectories is null || this._allowedDirectories.Count == 0) @@ -159,8 +174,6 @@ private bool IsFilePathAllowed(string path) return false; } - var canonicalDir = Path.GetFullPath(directoryPath); - foreach (var allowedDirectory in this._allowedDirectories) { var canonicalAllowed = Path.GetFullPath(allowedDirectory); @@ -170,8 +183,8 @@ private bool IsFilePathAllowed(string path) canonicalAllowed += separator; } - if (canonicalDir.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase) - || (canonicalDir + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase)) + if (directoryPath.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase) + || (directoryPath + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase)) { return true; } diff --git a/dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs b/dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs index fd708eb24af1..2fef0ae8db2d 100644 --- a/dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs +++ b/dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs @@ -32,7 +32,7 @@ public Task GetFileContentStreamAsync(string filePath, CancellationToken { try { - return Task.FromResult(File.Open(Environment.ExpandEnvironmentVariables(filePath), FileMode.Open, FileAccess.Read)); + return Task.FromResult(File.Open(filePath, FileMode.Open, FileAccess.Read)); } catch (Exception e) { @@ -58,7 +58,7 @@ public Task GetWriteableFileStreamAsync(string filePath, CancellationTok { try { - return Task.FromResult(File.Open(Environment.ExpandEnvironmentVariables(filePath), FileMode.Open, FileAccess.ReadWrite)); + return Task.FromResult(File.Open(filePath, FileMode.Open, FileAccess.ReadWrite)); } catch (Exception e) { @@ -82,7 +82,7 @@ public Task CreateFileAsync(string filePath, CancellationToken cancellat { try { - return Task.FromResult(File.Create(Environment.ExpandEnvironmentVariables(filePath))); + return Task.FromResult(File.Create(filePath)); } catch (Exception e) { @@ -95,7 +95,7 @@ public Task FileExistsAsync(string filePath, CancellationToken cancellatio { try { - return Task.FromResult(File.Exists(Environment.ExpandEnvironmentVariables(filePath))); + return Task.FromResult(File.Exists(filePath)); } catch (Exception e) { diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs index d68b9833847a..9bd334b62f01 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs @@ -247,7 +247,66 @@ public async Task ItDeniesRelativePathsAsync() AllowedDirectories = [Path.GetTempPath()] }; - // Act & Assert — relative paths are caught by the "fully qualified" check - await Assert.ThrowsAsync(async () => await target.ReadTextAsync("myfile.docx")); + // Act & Assert — relative paths resolve to CWD after canonicalization, + // which will be outside the allowed directories + await Assert.ThrowsAsync(async () => await target.ReadTextAsync("myfile.docx")); + } + + [Fact] + public async Task ItDeniesEnvVarExpansionBypassOnReadAsync() + { + // Arrange — path that starts with an allowed directory but uses env-var expansion + // to escape to a different location after expansion. + var allowedFolder = Path.Combine(Path.GetTempPath(), "allowed-sandbox"); + // Construct a path like C:\temp\allowed-sandbox\..%HOMEPATH%\secret.docx + // After env-var expansion, %HOMEPATH% becomes \Users\, and ".." traverses out. + var maliciousPath = Path.Combine(allowedFolder, "..%HOMEPATH%", "secret.docx"); + + var fileSystemConnectorMock = new Mock(); + var documentConnectorMock = new Mock(); + var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + { + AllowedDirectories = [allowedFolder] + }; + + // Act & Assert — the path should be denied because env vars are expanded + // before validation, so the canonical path lands outside the allowed directory. + await Assert.ThrowsAsync(async () => await target.ReadTextAsync(maliciousPath)); + } + + [Fact] + public async Task ItDeniesEnvVarExpansionBypassOnWriteAsync() + { + // Arrange + var allowedFolder = Path.Combine(Path.GetTempPath(), "allowed-sandbox"); + var maliciousPath = Path.Combine(allowedFolder, "..%HOMEPATH%", "secret.docx"); + + var fileSystemConnectorMock = new Mock(); + var documentConnectorMock = new Mock(); + var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + { + AllowedDirectories = [allowedFolder] + }; + + // Act & Assert + await Assert.ThrowsAsync(async () => await target.AppendTextAsync("text", maliciousPath)); + } + + [Fact] + public async Task ItDeniesPercentEncodedPathTraversalOnReadAsync() + { + // Arrange — paths containing bare %VAR% that would expand to absolute paths + var allowedFolder = Path.Combine(Path.GetTempPath(), "sandbox"); + var maliciousPath = "%APPDATA%\\secret.docx"; + + var fileSystemConnectorMock = new Mock(); + var documentConnectorMock = new Mock(); + var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + { + AllowedDirectories = [allowedFolder] + }; + + // Act & Assert — after env-var expansion, the path resolves outside the sandbox + await Assert.ThrowsAsync(async () => await target.ReadTextAsync(maliciousPath)); } } From 60c4e5c68bacfb836e43b086a8d2ab2e10f69082 Mon Sep 17 00:00:00 2001 From: SergeyMenshykh Date: Wed, 6 May 2026 14:29:57 +0100 Subject: [PATCH 2/2] Address PR review feedback - Validate UNC paths after env-var expansion, not just before - Use OS-appropriate case comparison for path validation - Make tests cross-platform using test-specific env vars and Path APIs - Test relative paths for both read and write operations - Use unique allowed folder in relative path test for determinism Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../Plugins.Document/DocumentPlugin.cs | 21 ++- .../Document/DocumentPluginTests.cs | 135 +++++++++++++----- 2 files changed, 117 insertions(+), 39 deletions(-) diff --git a/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs b/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs index 0bf8aec91a15..2cc87da09a48 100644 --- a/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs @@ -4,6 +4,7 @@ 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; @@ -153,9 +154,23 @@ private static string CanonicalizePath(string path) // Expand environment variables first, then canonicalize — so that // validation and I/O operate on the same resolved path. var expanded = Environment.ExpandEnvironmentVariables(path); + + // Re-check after expansion: an env var could have expanded to a UNC + // or extended-path prefix (e.g., %NETSHARE% → \\server\share). + if (expanded.StartsWith("\\\\", StringComparison.OrdinalIgnoreCase)) + { + throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); + } + return Path.GetFullPath(expanded); } + // 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; + /// /// Checks whether a canonicalized file path falls within one of the allowed directories. /// Subdirectories of allowed directories are also permitted. @@ -178,13 +193,13 @@ private bool IsFilePathAllowed(string canonicalPath) { var canonicalAllowed = Path.GetFullPath(allowedDirectory); var separator = Path.DirectorySeparatorChar.ToString(); - if (!canonicalAllowed.EndsWith(separator, StringComparison.OrdinalIgnoreCase)) + if (!canonicalAllowed.EndsWith(separator, s_pathComparison)) { canonicalAllowed += separator; } - if (directoryPath.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase) - || (directoryPath + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase)) + if (directoryPath.StartsWith(canonicalAllowed, s_pathComparison) + || (directoryPath + separator).Equals(canonicalAllowed, s_pathComparison)) { return true; } diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs index 9bd334b62f01..1308485da8e8 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs @@ -239,74 +239,137 @@ public async Task ItAllowsSubdirectoriesOfAllowedFoldersAsync() [Fact] public async Task ItDeniesRelativePathsAsync() { - // Arrange + // Arrange — use a unique subfolder so this test is deterministic regardless of CWD + var allowedFolder = Path.Combine(Path.GetTempPath(), "unique-allowed-" + Guid.NewGuid().ToString("N")[..8]); var fileSystemConnectorMock = new Mock(); var documentConnectorMock = new Mock(); var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) { - AllowedDirectories = [Path.GetTempPath()] + AllowedDirectories = [allowedFolder] }; // Act & Assert — relative paths resolve to CWD after canonicalization, // which will be outside the allowed directories await Assert.ThrowsAsync(async () => await target.ReadTextAsync("myfile.docx")); + await Assert.ThrowsAsync(async () => await target.AppendTextAsync("text", "myfile.docx")); } [Fact] public async Task ItDeniesEnvVarExpansionBypassOnReadAsync() { - // Arrange — path that starts with an allowed directory but uses env-var expansion - // to escape to a different location after expansion. + // Arrange — use a test-specific env var that expands to a value containing + // a path separator + ".." which creates a traversal after expansion. var allowedFolder = Path.Combine(Path.GetTempPath(), "allowed-sandbox"); - // Construct a path like C:\temp\allowed-sandbox\..%HOMEPATH%\secret.docx - // After env-var expansion, %HOMEPATH% becomes \Users\, and ".." traverses out. - var maliciousPath = Path.Combine(allowedFolder, "..%HOMEPATH%", "secret.docx"); + var envVarName = "SK_TEST_EXPAND_" + Guid.NewGuid().ToString("N")[..8]; - var fileSystemConnectorMock = new Mock(); - var documentConnectorMock = new Mock(); - var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + try { - AllowedDirectories = [allowedFolder] - }; - - // Act & Assert — the path should be denied because env vars are expanded - // before validation, so the canonical path lands outside the allowed directory. - await Assert.ThrowsAsync(async () => await target.ReadTextAsync(maliciousPath)); + // The env var value starts with a separator + ".." so that after expansion + // the path becomes: allowed-sandbox/..elsewheresecret.docx + Environment.SetEnvironmentVariable(envVarName, + $"{Path.DirectorySeparatorChar}..{Path.DirectorySeparatorChar}elsewhere"); + var maliciousPath = Path.Combine(allowedFolder, $"%{envVarName}%", "secret.docx"); + + var fileSystemConnectorMock = new Mock(); + var documentConnectorMock = new Mock(); + var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + { + AllowedDirectories = [allowedFolder] + }; + + // Act & Assert — the path should be denied because env vars are expanded + // before validation, so the canonical path lands outside the allowed directory. + await Assert.ThrowsAsync(async () => await target.ReadTextAsync(maliciousPath)); + } + finally + { + Environment.SetEnvironmentVariable(envVarName, null); + } } [Fact] public async Task ItDeniesEnvVarExpansionBypassOnWriteAsync() { - // Arrange + // Arrange — same pattern as read test, for the write path. var allowedFolder = Path.Combine(Path.GetTempPath(), "allowed-sandbox"); - var maliciousPath = Path.Combine(allowedFolder, "..%HOMEPATH%", "secret.docx"); + var envVarName = "SK_TEST_EXPAND_W_" + Guid.NewGuid().ToString("N")[..8]; - var fileSystemConnectorMock = new Mock(); - var documentConnectorMock = new Mock(); - var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + try { - AllowedDirectories = [allowedFolder] - }; - - // Act & Assert - await Assert.ThrowsAsync(async () => await target.AppendTextAsync("text", maliciousPath)); + Environment.SetEnvironmentVariable(envVarName, + $"{Path.DirectorySeparatorChar}..{Path.DirectorySeparatorChar}elsewhere"); + var maliciousPath = Path.Combine(allowedFolder, $"%{envVarName}%", "secret.docx"); + + var fileSystemConnectorMock = new Mock(); + var documentConnectorMock = new Mock(); + var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + { + AllowedDirectories = [allowedFolder] + }; + + // Act & Assert + await Assert.ThrowsAsync(async () => await target.AppendTextAsync("text", maliciousPath)); + } + finally + { + Environment.SetEnvironmentVariable(envVarName, null); + } } [Fact] - public async Task ItDeniesPercentEncodedPathTraversalOnReadAsync() + public async Task ItDeniesEnvVarExpansionToAbsolutePathAsync() { - // Arrange — paths containing bare %VAR% that would expand to absolute paths + // Arrange — env var that expands to an absolute path outside the sandbox var allowedFolder = Path.Combine(Path.GetTempPath(), "sandbox"); - var maliciousPath = "%APPDATA%\\secret.docx"; + var envVarName = "SK_TEST_ABS_" + Guid.NewGuid().ToString("N")[..8]; + var outsidePath = Path.Combine(Path.GetTempPath(), "outside"); - var fileSystemConnectorMock = new Mock(); - var documentConnectorMock = new Mock(); - var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + try { - AllowedDirectories = [allowedFolder] - }; + Environment.SetEnvironmentVariable(envVarName, outsidePath); + var maliciousPath = Path.Combine($"%{envVarName}%", "secret.docx"); + + var fileSystemConnectorMock = new Mock(); + var documentConnectorMock = new Mock(); + var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + { + AllowedDirectories = [allowedFolder] + }; + + // Act & Assert — after env-var expansion, the path resolves outside the sandbox + await Assert.ThrowsAsync(async () => await target.ReadTextAsync(maliciousPath)); + } + finally + { + Environment.SetEnvironmentVariable(envVarName, null); + } + } + + [Fact] + public async Task ItDeniesUncPathsIntroducedViaEnvVarExpansionAsync() + { + // Arrange — env var that expands to a UNC path + var allowedFolder = Path.Combine(Path.GetTempPath(), "sandbox"); + var envVarName = "SK_TEST_UNC_" + Guid.NewGuid().ToString("N")[..8]; - // Act & Assert — after env-var expansion, the path resolves outside the sandbox - await Assert.ThrowsAsync(async () => await target.ReadTextAsync(maliciousPath)); + try + { + Environment.SetEnvironmentVariable(envVarName, @"\\evil-server\share"); + var maliciousPath = $"%{envVarName}%{Path.DirectorySeparatorChar}secret.docx"; + + var fileSystemConnectorMock = new Mock(); + var documentConnectorMock = new Mock(); + var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object) + { + AllowedDirectories = [allowedFolder] + }; + + // Act & Assert — expanded path is UNC, should be rejected + await Assert.ThrowsAsync(async () => await target.ReadTextAsync(maliciousPath)); + } + finally + { + Environment.SetEnvironmentVariable(envVarName, null); + } } }