.Net: Fix DocumentPlugin path validation order

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;
@@ -87,14 +88,16 @@ public async Task<string> 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 +110,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 +139,10 @@ public async Task AppendTextAsync(
     private HashSet<string>? _allowedDirectories = [];
 
     /// <summary>
-    /// 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.
     /// </summary>
-    private bool IsFilePathAllowed(string path)
+    private static string CanonicalizePath(string path)
     {
         Verify.NotNullOrWhiteSpace(path);
 
@@ -147,31 +151,55 @@ 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);
+
+        // 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;
+
+    /// <summary>
+    /// Checks whether a canonicalized file path falls within one of the allowed directories.
+    /// Subdirectories of allowed directories are also permitted.
+    /// </summary>
+    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)
         {
             return false;
         }
 
-        var canonicalDir = Path.GetFullPath(directoryPath);
-
         foreach (var allowedDirectory in this._allowedDirectories)
         {
             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 (canonicalDir.StartsWith(canonicalAllowed, StringComparison.OrdinalIgnoreCase)
-                || (canonicalDir + separator).Equals(canonicalAllowed, StringComparison.OrdinalIgnoreCase))
+            if (directoryPath.StartsWith(canonicalAllowed, s_pathComparison)
+                || (directoryPath + separator).Equals(canonicalAllowed, s_pathComparison))
             {
                 return true;
             }

dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs

@@ -32,7 +32,7 @@ public Task<Stream> GetFileContentStreamAsync(string filePath, CancellationToken
     {
         try
         {
-            return Task.FromResult<Stream>(File.Open(Environment.ExpandEnvironmentVariables(filePath), FileMode.Open, FileAccess.Read));
+            return Task.FromResult<Stream>(File.Open(filePath, FileMode.Open, FileAccess.Read));
         }
         catch (Exception e)
         {
@@ -58,7 +58,7 @@ public Task<Stream> GetWriteableFileStreamAsync(string filePath, CancellationTok
     {
         try
         {
-            return Task.FromResult<Stream>(File.Open(Environment.ExpandEnvironmentVariables(filePath), FileMode.Open, FileAccess.ReadWrite));
+            return Task.FromResult<Stream>(File.Open(filePath, FileMode.Open, FileAccess.ReadWrite));
         }
         catch (Exception e)
         {
@@ -82,7 +82,7 @@ public Task<Stream> CreateFileAsync(string filePath, CancellationToken cancellat
     {
         try
         {
-            return Task.FromResult<Stream>(File.Create(Environment.ExpandEnvironmentVariables(filePath)));
+            return Task.FromResult<Stream>(File.Create(filePath));
         }
         catch (Exception e)
         {
@@ -95,7 +95,7 @@ public Task<bool> FileExistsAsync(string filePath, CancellationToken cancellatio
     {
         try
         {
-            return Task.FromResult(File.Exists(Environment.ExpandEnvironmentVariables(filePath)));
+            return Task.FromResult(File.Exists(filePath));
         }
         catch (Exception e)
         {

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

@@ -239,15 +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<IFileSystemConnector>();
         var documentConnectorMock = new Mock<IDocumentConnector>();
         var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
         {
-            AllowedDirectories = [Path.GetTempPath()]
+            AllowedDirectories = [allowedFolder]
         };
 
-        // Act & Assert — relative paths are caught by the "fully qualified" check
-        await Assert.ThrowsAsync<ArgumentException>(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<InvalidOperationException>(async () => await target.ReadTextAsync("myfile.docx"));
+        await Assert.ThrowsAsync<InvalidOperationException>(async () => await target.AppendTextAsync("text", "myfile.docx"));
+    }
+
+    [Fact]
+    public async Task ItDeniesEnvVarExpansionBypassOnReadAsync()
+    {
+        // 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");
+        var envVarName = "SK_TEST_EXPAND_" + Guid.NewGuid().ToString("N")[..8];
+
+        try
+        {
+            // The env var value starts with a separator + ".." so that after expansion
+            // the path becomes: allowed-sandbox/<sep>..<sep>elsewhere<sep>secret.docx
+            Environment.SetEnvironmentVariable(envVarName,
+                $"{Path.DirectorySeparatorChar}..{Path.DirectorySeparatorChar}elsewhere");
+            var maliciousPath = Path.Combine(allowedFolder, $"%{envVarName}%", "secret.docx");
+
+            var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
+            var documentConnectorMock = new Mock<IDocumentConnector>();
+            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<InvalidOperationException>(async () => await target.ReadTextAsync(maliciousPath));
+        }
+        finally
+        {
+            Environment.SetEnvironmentVariable(envVarName, null);
+        }
+    }
+
+    [Fact]
+    public async Task ItDeniesEnvVarExpansionBypassOnWriteAsync()
+    {
+        // Arrange — same pattern as read test, for the write path.
+        var allowedFolder = Path.Combine(Path.GetTempPath(), "allowed-sandbox");
+        var envVarName = "SK_TEST_EXPAND_W_" + Guid.NewGuid().ToString("N")[..8];
+
+        try
+        {
+            Environment.SetEnvironmentVariable(envVarName,
+                $"{Path.DirectorySeparatorChar}..{Path.DirectorySeparatorChar}elsewhere");
+            var maliciousPath = Path.Combine(allowedFolder, $"%{envVarName}%", "secret.docx");
+
+            var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
+            var documentConnectorMock = new Mock<IDocumentConnector>();
+            var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
+            {
+                AllowedDirectories = [allowedFolder]
+            };
+
+            // Act & Assert
+            await Assert.ThrowsAsync<InvalidOperationException>(async () => await target.AppendTextAsync("text", maliciousPath));
+        }
+        finally
+        {
+            Environment.SetEnvironmentVariable(envVarName, null);
+        }
+    }
+
+    [Fact]
+    public async Task ItDeniesEnvVarExpansionToAbsolutePathAsync()
+    {
+        // Arrange — env var that expands to an absolute path outside the sandbox
+        var allowedFolder = Path.Combine(Path.GetTempPath(), "sandbox");
+        var envVarName = "SK_TEST_ABS_" + Guid.NewGuid().ToString("N")[..8];
+        var outsidePath = Path.Combine(Path.GetTempPath(), "outside");
+
+        try
+        {
+            Environment.SetEnvironmentVariable(envVarName, outsidePath);
+            var maliciousPath = Path.Combine($"%{envVarName}%", "secret.docx");
+
+            var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
+            var documentConnectorMock = new Mock<IDocumentConnector>();
+            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<InvalidOperationException>(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];
+
+        try
+        {
+            Environment.SetEnvironmentVariable(envVarName, @"\\evil-server\share");
+            var maliciousPath = $"%{envVarName}%{Path.DirectorySeparatorChar}secret.docx";
+
+            var fileSystemConnectorMock = new Mock<IFileSystemConnector>();
+            var documentConnectorMock = new Mock<IDocumentConnector>();
+            var target = new DocumentPlugin(documentConnectorMock.Object, fileSystemConnectorMock.Object)
+            {
+                AllowedDirectories = [allowedFolder]
+            };
+
+            // Act & Assert — expanded path is UNC, should be rejected
+            await Assert.ThrowsAsync<ArgumentException>(async () => await target.ReadTextAsync(maliciousPath));
+        }
+        finally
+        {
+            Environment.SetEnvironmentVariable(envVarName, null);
+        }
     }
 }