From 159d17569b863ad68d6835046fdf548a7498f923 Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Thu, 29 Oct 2026 16:27:06 +0000 Subject: [PATCH 1/7] Consolidate file access --- .../workflows/dotnet-secure-file-tests.yml | 33 + .../Demos/CodeInterpreterPlugin/Program.cs | 1 + .../src/System/SecureFileSystem.cs | 810 ++++++++++++++++++ .../CodeInterpreter/SessionsPythonPlugin.cs | 24 +- .../CodeInterpreter/SessionsPythonSettings.cs | 4 +- .../src/Plugins/Plugins.Core/FileIOPlugin.cs | 29 +- .../Plugins/Plugins.Core/Plugins.Core.csproj | 4 + .../Plugins.Document/DocumentPlugin.cs | 63 +- .../FileSystem/IFileSystemConnector.cs | 30 +- .../FileSystem/LocalFileSystemConnector.cs | 94 +- .../Plugins.MsGraph/CloudDrivePlugin.cs | 47 +- .../Connectors/OneDriveConnector.cs | 53 +- .../Plugins.MsGraph/ICloudDriveConnector.cs | 5 +- .../Core/SecureFileSystemTests.cs | 427 +++++++++ .../Core/SessionsPythonPluginTests.cs | 103 ++- .../Document/DocumentPluginTests.cs | 26 +- .../Document/LocalFileSystemConnectorTests.cs | 87 ++ .../MsGraph/CloudDrivePluginTests.cs | 64 +- .../MsGraph/OneDriveConnectorTests.cs | 69 ++ .../Plugins.UnitTests.csproj | 5 - .../Support/SymbolicLinkFactAttribute.cs | 64 ++ .../Web/WebFileDownloadPluginTests.cs | 123 +++ .../Plugins.Web/WebFileDownloadPlugin.cs | 44 +- 23 files changed, 1933 insertions(+), 276 deletions(-) create mode 100644 .github/workflows/dotnet-secure-file-tests.yml create mode 100644 dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs create mode 100644 dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs create mode 100644 dotnet/src/Plugins/Plugins.UnitTests/Document/LocalFileSystemConnectorTests.cs create mode 100644 dotnet/src/Plugins/Plugins.UnitTests/MsGraph/OneDriveConnectorTests.cs create mode 100644 dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs diff --git a/.github/workflows/dotnet-secure-file-tests.yml b/.github/workflows/dotnet-secure-file-tests.yml new file mode 100644 index 000000000000..32383fdf94bb --- /dev/null +++ b/.github/workflows/dotnet-secure-file-tests.yml @@ -0,0 +1,33 @@ +name: dotnet-secure-file-tests + +on: + workflow_dispatch: + pull_request: + paths: + - '.github/workflows/dotnet-secure-file-tests.yml' + - 'dotnet/src/InternalUtilities/**' + - 'dotnet/src/Plugins/**' + - 'dotnet/Directory.*.props' + - 'dotnet/global.json' + +permissions: + contents: read + +jobs: + test: + strategy: + fail-fast: false + matrix: + os: [ubuntu-latest, windows-latest, macos-latest] + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - uses: actions/setup-dotnet@26b0ec14cb23fa6904739307f278c14f94c95bf1 # v5.4.0 + with: + global-json-file: dotnet/global.json + - name: Test secure file operations + run: >- + dotnet test dotnet/src/Plugins/Plugins.UnitTests/Plugins.UnitTests.csproj + --filter "FullyQualifiedName~SecureFileSystemTests|FullyQualifiedName~FileIOPluginTests|FullyQualifiedName~SessionsPythonPluginTests|FullyQualifiedName~WebFileDownloadPluginTests|FullyQualifiedName~CloudDrivePluginTests|FullyQualifiedName~DocumentPluginTests|FullyQualifiedName~LocalFileSystemConnectorTests|FullyQualifiedName~OneDriveConnectorTests" diff --git a/dotnet/samples/Demos/CodeInterpreterPlugin/Program.cs b/dotnet/samples/Demos/CodeInterpreterPlugin/Program.cs index 35cd4c537ba2..bb835f7d226f 100644 --- a/dotnet/samples/Demos/CodeInterpreterPlugin/Program.cs +++ b/dotnet/samples/Demos/CodeInterpreterPlugin/Program.cs @@ -65,6 +65,7 @@ async Task TokenProvider(CancellationToken cancellationToken) // Uncomment the following lines to enable file upload operations (disabled by default for security) // settings.EnableDangerousFileUploads = true; // settings.AllowedUploadDirectories = new[] { @"C:\allowed\upload\directory" }; +// Local download saves also require an allowed directory; returning bytes does not. // settings.AllowedDownloadDirectories = new[] { @"C:\allowed\download\directory" }; Console.WriteLine("=== Code Interpreter With Azure Container Apps Plugin Demo ===\n"); diff --git a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs new file mode 100644 index 000000000000..fbf9044b96f2 --- /dev/null +++ b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs @@ -0,0 +1,810 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.IO; +using System.Runtime.InteropServices; +using System.Security; +using Microsoft.Win32.SafeHandles; + +namespace Microsoft.SemanticKernel; + +/// +/// Opens local files without separating path validation from file access. +/// The returned stream owns the same handle that was checked, so replacing a +/// validated path with a symbolic link cannot redirect the later read or write. +/// +/// +/// Allowed roots and their configuration must remain under trusted ownership. +/// This prevents link replacement, not changes to mounts, hard links, or the contents of an open file. +/// Linux file inspection requires libc/kernel support for statx; failure never falls back to a path check. +/// +internal static class SecureFileSystem +{ + internal static FileStream OpenRead( + string path, + IEnumerable? allowedDirectories, + out string resolvedPath, + bool rejectReadOnly = false, + FileShare share = FileShare.Read) + { + var allowedPath = GetAllowedPath(path, allowedDirectories); + resolvedPath = allowedPath.ResolvedPath; + return OpenRestrictedFile(allowedPath, FileMode.Open, FileAccess.Read, share, rejectReadOnly, out _); + } + + internal static FileStream OpenWrite( + string path, + IEnumerable? allowedDirectories, + out string resolvedPath, + FileMode mode = FileMode.Create, + FileShare share = FileShare.Read) + { + if (mode is not FileMode.Create and not FileMode.CreateNew) + { + throw new ArgumentOutOfRangeException(nameof(mode)); + } + + var allowedPath = GetAllowedPath(path, allowedDirectories); + resolvedPath = allowedPath.ResolvedPath; + return OpenRestrictedFile(allowedPath, mode, FileAccess.Write, share, rejectReadOnly: true, out _); + } + + internal static FileStream OpenUpdate( + string path, + IEnumerable? allowedDirectories, + out bool created) + { + var allowedPath = GetAllowedPath(path, allowedDirectories); + return OpenRestrictedFile( + allowedPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None, rejectReadOnly: true, out created); + } + +#pragma warning disable CA2000 // The returned FileStream owns the handle; failures dispose it below. + private static FileStream OpenRestrictedFile( + AllowedPath allowedPath, + FileMode mode, + FileAccess access, + FileShare share, + bool rejectReadOnly, + out bool created) + { + SafeFileHandle fileHandle; + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + fileHandle = WindowsFileSystem.Open( + allowedPath.AllowedDirectory, allowedPath.RelativeSegments, mode, access, share, rejectReadOnly, out created); + } + else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || + RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) + { + fileHandle = UnixFileSystem.Open( + allowedPath.AllowedDirectory, allowedPath.RelativeSegments, mode, access, share, rejectReadOnly, out created); + } + else + { + throw new PlatformNotSupportedException( + "Secure local file access is supported only on Windows, Linux, and macOS."); + } + + FileStream? stream = null; + try + { + stream = new FileStream(fileHandle, access); + if (mode == FileMode.Create) + { + // Do not truncate an existing file until the securely opened handle has passed every check. + stream.SetLength(0); + } + + return stream; + } + catch + { + stream?.Dispose(); + fileHandle.Dispose(); + throw; + } + } +#pragma warning restore CA2000 + + internal static string NormalizePath(string path, bool expandEnvironmentVariables = false) + { + Verify.NotNullOrWhiteSpace(path); + RejectNetworkOrDevicePath(path); + var expanded = expandEnvironmentVariables ? Environment.ExpandEnvironmentVariables(path) : path; + RejectNetworkOrDevicePath(expanded); + var fullPath = Path.GetFullPath(expanded); + RejectNetworkOrDevicePath(fullPath); + return fullPath; + } + + private static void RejectNetworkOrDevicePath(string path) + { + if (path.Length >= 2 && path[0] is '/' or '\\' && path[1] is '/' or '\\') + { + throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); + } + } + + private static AllowedPath GetAllowedPath(string path, IEnumerable? allowedDirectories) + { + var fullPath = NormalizePath(path); + if (allowedDirectories is null) + { + throw new InvalidOperationException("File access requires allowed directories to be configured."); + } + + using var enumerator = allowedDirectories.GetEnumerator(); + if (!enumerator.MoveNext()) + { + throw new InvalidOperationException("File access requires allowed directories to be configured."); + } + + // Canonicalization identifies the intended location. The native open below still walks + // every path part without following links because this string can become stale immediately. + string resolvedPath; + try + { + resolvedPath = PathUtilities.GetSafeFullPath(fullPath); + RejectNetworkOrDevicePath(resolvedPath); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or InvalidOperationException or SecurityException) + { + throw new InvalidOperationException("The requested file path is not allowed."); + } + + do + { + // Invalid trusted configuration must remain visible, unlike rejected caller paths. + var resolvedAllowedDirectory = TrimTrailingSeparators( + PathUtilities.GetSafeFullPath(NormalizePath(enumerator.Current))); + RejectNetworkOrDevicePath(resolvedAllowedDirectory); + var separator = Path.DirectorySeparatorChar.ToString(); + var allowedPrefix = resolvedAllowedDirectory.EndsWith(separator, PathUtilities.PathComparison) + ? resolvedAllowedDirectory + : resolvedAllowedDirectory + separator; + + if (!resolvedPath.StartsWith(allowedPrefix, PathUtilities.PathComparison)) + { + continue; + } + + var relativePath = resolvedPath.Substring(allowedPrefix.Length); + var relativeSegments = relativePath.Split( + new[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar }, + StringSplitOptions.RemoveEmptyEntries); + + if (relativeSegments.Length > 0) + { + return new AllowedPath(resolvedAllowedDirectory, resolvedPath, relativeSegments); + } + } + while (enumerator.MoveNext()); + + throw new InvalidOperationException("The requested file path is not within allowed directories."); + } + + private static string TrimTrailingSeparators(string path) + { + var rootLength = (Path.GetPathRoot(path) ?? string.Empty).Length; + var length = path.Length; + + while (length > rootLength && + (path[length - 1] == Path.DirectorySeparatorChar || + path[length - 1] == Path.AltDirectorySeparatorChar)) + { + length--; + } + + return length == path.Length ? path : path.Substring(0, length); + } + + private readonly struct AllowedPath( + string allowedDirectory, + string resolvedPath, + string[] relativeSegments) + { + internal string AllowedDirectory { get; } = allowedDirectory; + + internal string ResolvedPath { get; } = resolvedPath; + + internal string[] RelativeSegments { get; } = relativeSegments; + } + + private static class UnixFileSystem + { + private const int ReadOnly = 0; + private const int WriteOnly = 1; + private const int ReadWrite = 2; + private const uint DefaultPermissions = 0x1B6; // 0666, reduced by the process umask. + private const int AlreadyExists = 17; + +#pragma warning disable CA2000 // Ownership of the verified file handle is returned; failed checks dispose it. + internal static SafeFileHandle Open( + string allowedDirectory, + string[] relativeSegments, + FileMode mode, + FileAccess access, + FileShare share, + bool rejectReadOnly, + out bool created) + { + created = false; + var directoryFlags = GetDirectoryFlags(); + var currentHandle = OpenPath("/", directoryFlags, 0); + + try + { + // Start at the filesystem root and open each directory relative to its parent. + // This keeps every parent stable even if its name is replaced during the walk. + foreach (var segment in GetSegmentsAfterRoot(allowedDirectory)) + { + var nextHandle = OpenAt(currentHandle, segment, directoryFlags, 0); + currentHandle.Dispose(); + currentHandle = nextHandle; + } + + for (var i = 0; i < relativeSegments.Length - 1; i++) + { + var nextHandle = OpenAt(currentHandle, relativeSegments[i], directoryFlags, 0); + currentHandle.Dispose(); + currentHandle = nextHandle; + } + + var name = relativeSegments[relativeSegments.Length - 1]; + var fileFlags = (access == FileAccess.Read ? ReadOnly : access == FileAccess.Write ? WriteOnly : ReadWrite) | + GetNoFollowFlag() | GetCloseOnExecFlag() | GetNonBlockingFlag(); + + SafeFileHandle handle; + if (mode == FileMode.OpenOrCreate) + { + // Exclusive creation supplies reliable creation status. A conflicting file is + // opened without truncation; if it vanishes again we fail rather than spin. + var descriptor = OpenAtNative(currentHandle, name, + fileFlags | GetCreateFlag() | GetExclusiveFlag(), DefaultPermissions); + if (descriptor >= 0) + { + handle = new SafeFileHandle(new IntPtr(descriptor), ownsHandle: true); + created = true; + } + else if (Marshal.GetLastWin32Error() == AlreadyExists) + { + handle = OpenAt(currentHandle, name, fileFlags, 0); + } + else + { + return CreateHandle(descriptor, name); + } + } + else + { + if (mode is FileMode.Create or FileMode.CreateNew) + { + fileFlags |= GetCreateFlag(); + } + + if (mode == FileMode.CreateNew) + { + fileFlags |= GetExclusiveFlag(); + } + + handle = OpenAt(currentHandle, name, fileFlags, DefaultPermissions); + created = mode == FileMode.CreateNew; + } + + try + { + // Nonblocking open prevents a substituted pipe from hanging before we can + // inspect the handle. Only ordinary files may reach the returned FileStream. + CheckFile(handle, rejectReadOnly); + if (Flock(handle, (share == FileShare.None ? 2 : 1) | 4) != 0) + { + throw new IOException("The file is in use."); + } + + return handle; + } + catch + { + handle.Dispose(); + throw; + } + } + finally + { + currentHandle.Dispose(); + } + } +#pragma warning restore CA2000 + + private static int GetDirectoryFlags() => + ReadOnly | GetDirectoryFlag() | GetNoFollowFlag() | GetCloseOnExecFlag(); + + private static int GetDirectoryFlag() => + RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x100000 : 0x10000; + + private static int GetCreateFlag() => + RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x200 : 0x40; + + private static int GetExclusiveFlag() => + RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x800 : 0x80; + + private static int GetNonBlockingFlag() => + RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x4 : 0x800; + + private static int GetNoFollowFlag() => + RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x100 : 0x20000; + + private static int GetCloseOnExecFlag() => + RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x1000000 : 0x80000; + + private static string[] GetSegmentsAfterRoot(string path) + { + var root = Path.GetPathRoot(path) ?? string.Empty; + return path.Substring(root.Length).Split( + new[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar }, + StringSplitOptions.RemoveEmptyEntries); + } + + private static SafeFileHandle OpenPath(string path, int flags, uint mode) + { + var descriptor = OpenNative(path, flags, mode); + return CreateHandle(descriptor, path); + } + + private static SafeFileHandle OpenAt(SafeFileHandle directory, string path, int flags, uint mode) + { + var descriptor = OpenAtNative(directory, path, flags, mode); + return CreateHandle(descriptor, path); + } + + private static void CheckFile(SafeFileHandle handle, bool rejectReadOnly) + { + ushort mode; + if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) + { + // statx has the same layout on all Linux architectures, unlike struct stat. + if (StatX(handle, string.Empty, 0x1000, 0x3, out var information) != 0) + { + throw new IOException($"Could not inspect the open file. Native error: {Marshal.GetLastWin32Error()}."); + } + + if ((information.Mask & 0x3) != 0x3) + { + throw new IOException("The filesystem did not supply the file type and permissions."); + } + + mode = information.Mode; + } + else + { + var result = RuntimeInformation.ProcessArchitecture == Architecture.X64 + ? StatMacX64(handle, out var information) + : StatMac(handle, out information); + if (result != 0) + { + throw new IOException($"Could not inspect the open file. Native error: {Marshal.GetLastWin32Error()}."); + } + + mode = information.Mode; + } + + if ((mode & 0xF000) != 0x8000) + { + throw new InvalidOperationException("Access denied: the target is not a regular file."); + } + + if (rejectReadOnly && (mode & 0x92) == 0) // No owner, group, or other write permission. + { + throw new UnauthorizedAccessException("File is read-only."); + } + } + + private static SafeFileHandle CreateHandle(int descriptor, string path) + { + if (descriptor < 0) + { + var error = Marshal.GetLastWin32Error(); + if (error == 2) + { + throw new FileNotFoundException($"Could not securely open '{path}'.", path); + } + + if (error == AlreadyExists) + { + throw new FileAlreadyExistsException(); + } + + if (error is 1 or 13 or 30) + { + throw new UnauthorizedAccessException("Access to the file was denied."); + } + + throw new InvalidOperationException( + $"Access denied: '{path}' could not be opened safely. Native error: {error}."); + } + + return new SafeFileHandle(new IntPtr(descriptor), ownsHandle: true); + } + + [DllImport("libc", EntryPoint = "open", SetLastError = true)] + private static extern int OpenNative(string path, int flags, uint mode); + + [DllImport("libc", EntryPoint = "openat", SetLastError = true)] + private static extern int OpenAtNative(SafeFileHandle directoryDescriptor, string path, int flags, uint mode); + + [DllImport("libc", EntryPoint = "flock", SetLastError = true)] + private static extern int Flock(SafeFileHandle descriptor, int operation); + + [DllImport("libc", EntryPoint = "statx", SetLastError = true)] + private static extern int StatX(SafeFileHandle descriptor, string path, int flags, uint mask, out LinuxStatX information); + + [DllImport("libc", EntryPoint = "fstat", SetLastError = true)] + private static extern int StatMac(SafeFileHandle descriptor, out MacStat information); + + [DllImport("libc", EntryPoint = "fstat$INODE64", SetLastError = true)] + private static extern int StatMacX64(SafeFileHandle descriptor, out MacStat information); + + [StructLayout(LayoutKind.Explicit, Size = 256)] + private struct LinuxStatX + { + [FieldOffset(0)] + internal uint Mask; + [FieldOffset(28)] + internal ushort Mode; + } + + [StructLayout(LayoutKind.Explicit, Size = 144)] + private struct MacStat + { + [FieldOffset(4)] + internal ushort Mode; + } + } + +#pragma warning disable RCS1194 // Internal marker for the atomic create-only conflict, with a fixed message. + internal sealed class FileAlreadyExistsException : IOException + { + internal FileAlreadyExistsException() : base("Overwriting existing files is disabled.") + { + } + } +#pragma warning restore RCS1194 + + private static class WindowsFileSystem + { + private const uint GenericRead = 0x80000000; + private const uint GenericWrite = 0x40000000; + private const uint FileListDirectory = 0x00000001; + private const uint FileReadAttributes = 0x00000080; + private const uint Synchronize = 0x00100000; + private const uint FileShareRead = 0x00000001; + private const uint FileShareWrite = 0x00000002; + private const uint FileShareDelete = 0x00000004; + private const uint OpenExisting = 3; + private const uint FileOpen = 1; + private const uint FileCreate = 2; + private const uint FileOpenIf = 3; + private const uint FileAttributeNormal = 0x00000080; + private const uint FileAttributeReparsePoint = 0x00000400; + private const uint FileFlagBackupSemantics = 0x02000000; + private const uint FileFlagOpenReparsePoint = 0x00200000; + private const uint FileDirectoryFile = 0x00000001; + private const uint FileSynchronousIoNonAlert = 0x00000020; + private const uint FileNonDirectoryFile = 0x00000040; + private const uint ObjectCaseInsensitive = 0x00000040; + private const int StatusObjectNameNotFound = unchecked((int)0xC0000034); + private const int StatusObjectPathNotFound = unchecked((int)0xC000003A); + private const int StatusObjectNameCollision = unchecked((int)0xC0000035); + private const int StatusAccessDenied = unchecked((int)0xC0000022); + + internal static SafeFileHandle Open( + string allowedDirectory, + string[] relativeSegments, + FileMode mode, + FileAccess access, + FileShare share, + bool rejectReadOnly, + out bool created) + { + var rootPath = Path.GetPathRoot(allowedDirectory) + ?? throw new InvalidOperationException($"Access denied: '{allowedDirectory}' has no path root."); + + var currentHandle = OpenRoot(rootPath); + + try + { + // Handle-relative opens prevent a checked parent directory from being swapped + // before the next path part is opened. + foreach (var segment in GetSegmentsAfterRoot(allowedDirectory, rootPath)) + { + var nextHandle = OpenRelativeDirectory(currentHandle, segment); + currentHandle.Dispose(); + currentHandle = nextHandle; + } + + for (var i = 0; i < relativeSegments.Length - 1; i++) + { + var nextHandle = OpenRelativeDirectory(currentHandle, relativeSegments[i]); + currentHandle.Dispose(); + currentHandle = nextHandle; + } + + return OpenRelativeFile( + currentHandle, + relativeSegments[relativeSegments.Length - 1], + mode, access, share, rejectReadOnly, out created); + } + finally + { + currentHandle.Dispose(); + } + } + + private static SafeFileHandle OpenRoot(string rootPath) + { + var handle = CreateFile( + rootPath, + FileListDirectory | FileReadAttributes | Synchronize, + FileShareRead | FileShareWrite | FileShareDelete, + IntPtr.Zero, + OpenExisting, + FileFlagBackupSemantics | FileFlagOpenReparsePoint, + IntPtr.Zero); + + if (handle.IsInvalid) + { + var error = Marshal.GetLastWin32Error(); + handle.Dispose(); + throw new InvalidOperationException( + $"Access denied: path root '{rootPath}' could not be opened safely. Native error: {error}."); + } + + ThrowIfReparsePoint(handle, rootPath); + return handle; + } + + private static SafeFileHandle OpenRelativeDirectory(SafeFileHandle parent, string segment) + { + var handle = OpenRelative( + parent, + segment, + FileListDirectory | FileReadAttributes | Synchronize, + FileOpen, + FileDirectoryFile | FileSynchronousIoNonAlert, + FileShareRead, + out _); + + ThrowIfReparsePoint(handle, segment); + return handle; + } + + private static SafeFileHandle OpenRelativeFile( + SafeFileHandle parent, + string segment, + FileMode mode, + FileAccess access, + FileShare share, + bool rejectReadOnly, + out bool created) + { + var desiredAccess = (access == FileAccess.Read ? GenericRead : + access == FileAccess.Write ? GenericWrite : GenericRead | GenericWrite) | + FileReadAttributes | + Synchronize; + var disposition = mode == FileMode.Open ? FileOpen : + mode == FileMode.CreateNew ? FileCreate : FileOpenIf; + + var handle = OpenRelative( + parent, + segment, + desiredAccess, + disposition, + FileNonDirectoryFile | FileSynchronousIoNonAlert, + (uint)share, + out created); + + // Opening the reparse point itself lets us reject it before any file content is changed. + try + { + var attributes = ThrowIfReparsePoint(handle, segment); + if (GetFileType(handle) != 1 || (attributes & 0x10) != 0) + { + throw new InvalidOperationException("Access denied: the target is not a regular file."); + } + + if (rejectReadOnly && (attributes & 1) != 0) + { + throw new UnauthorizedAccessException("File is read-only."); + } + + return handle; + } + catch + { + handle.Dispose(); + throw; + } + } + + private static SafeFileHandle OpenRelative( + SafeFileHandle parent, + string segment, + uint desiredAccess, + uint disposition, + uint options, + uint share, + out bool created) + { + var stringBuffer = Marshal.StringToHGlobalUni(segment); + var unicodeStringPointer = IntPtr.Zero; + + try + { + var unicodeString = new UnicodeString + { + Length = checked((ushort)(segment.Length * sizeof(char))), + MaximumLength = checked((ushort)((segment.Length + 1) * sizeof(char))), + Buffer = stringBuffer, + }; + + unicodeStringPointer = Marshal.AllocHGlobal(Marshal.SizeOf()); + Marshal.StructureToPtr(unicodeString, unicodeStringPointer, fDeleteOld: false); + + var objectAttributes = new ObjectAttributes + { + Length = Marshal.SizeOf(), + RootDirectory = parent.DangerousGetHandle(), + ObjectName = unicodeStringPointer, + Attributes = ObjectCaseInsensitive, + }; + + var status = NtCreateFile( + out var rawHandle, + desiredAccess, + ref objectAttributes, + out var ioStatus, + IntPtr.Zero, + FileAttributeNormal, + share, + disposition, + options | FileFlagOpenReparsePoint, + IntPtr.Zero, + 0); + + if (status < 0) + { + if (status == StatusObjectNameCollision) + { + throw new FileAlreadyExistsException(); + } + + if (status == StatusAccessDenied) + { + throw new UnauthorizedAccessException("Access to the file was denied."); + } + + if (status == StatusObjectNameNotFound || status == StatusObjectPathNotFound) + { + throw new FileNotFoundException($"Could not securely open '{segment}'.", segment); + } + + throw new InvalidOperationException( + $"Access denied: '{segment}' could not be opened safely. NTSTATUS: 0x{status:X8}."); + } + + created = ioStatus.Information == new IntPtr(2); + return new SafeFileHandle(rawHandle, ownsHandle: true); + } + finally + { + if (unicodeStringPointer != IntPtr.Zero) + { + Marshal.FreeHGlobal(unicodeStringPointer); + } + + Marshal.FreeHGlobal(stringBuffer); + } + } + + private static uint ThrowIfReparsePoint(SafeFileHandle handle, string segment) + { + if (!GetFileInformationByHandle(handle, out var information)) + { + var error = Marshal.GetLastWin32Error(); + handle.Dispose(); + throw new InvalidOperationException( + $"Access denied: '{segment}' could not be inspected safely. Native error: {error}."); + } + + if ((information.FileAttributes & FileAttributeReparsePoint) != 0) + { + handle.Dispose(); + throw new InvalidOperationException( + $"Access denied: path contains a symbolic link or reparse point at '{segment}'."); + } + + return information.FileAttributes; + } + + private static string[] GetSegmentsAfterRoot(string path, string root) => + path.Substring(root.Length).Split( + new[] { Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar }, + StringSplitOptions.RemoveEmptyEntries); + + [StructLayout(LayoutKind.Sequential)] + private struct UnicodeString + { + internal ushort Length; + internal ushort MaximumLength; + internal IntPtr Buffer; + } + + [StructLayout(LayoutKind.Sequential)] + private struct ObjectAttributes + { + internal int Length; + internal IntPtr RootDirectory; + internal IntPtr ObjectName; + internal uint Attributes; + internal IntPtr SecurityDescriptor; + internal IntPtr SecurityQualityOfService; + } + + [StructLayout(LayoutKind.Sequential)] + private struct IoStatusBlock + { + internal IntPtr Status; + internal IntPtr Information; + } + + [StructLayout(LayoutKind.Sequential)] + private struct ByHandleFileInformation + { + internal uint FileAttributes; + internal System.Runtime.InteropServices.ComTypes.FILETIME CreationTime; + internal System.Runtime.InteropServices.ComTypes.FILETIME LastAccessTime; + internal System.Runtime.InteropServices.ComTypes.FILETIME LastWriteTime; + internal uint VolumeSerialNumber; + internal uint FileSizeHigh; + internal uint FileSizeLow; + internal uint NumberOfLinks; + internal uint FileIndexHigh; + internal uint FileIndexLow; + } + + [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + private static extern SafeFileHandle CreateFile( + string fileName, + uint desiredAccess, + uint shareMode, + IntPtr securityAttributes, + uint creationDisposition, + uint flagsAndAttributes, + IntPtr templateFile); + + [DllImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool GetFileInformationByHandle( + SafeFileHandle file, + out ByHandleFileInformation fileInformation); + + [DllImport("kernel32.dll", SetLastError = true)] + private static extern uint GetFileType(SafeFileHandle file); + + [DllImport("ntdll.dll")] + private static extern int NtCreateFile( + out IntPtr fileHandle, + uint desiredAccess, + ref ObjectAttributes objectAttributes, + out IoStatusBlock ioStatusBlock, + IntPtr allocationSize, + uint fileAttributes, + uint shareAccess, + uint createDisposition, + uint createOptions, + IntPtr eaBuffer, + uint eaLength); + } +} diff --git a/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs b/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs index 681298cd0dc7..21a5f3b4951f 100644 --- a/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs @@ -134,7 +134,11 @@ public async Task UploadFileAsync( using var httpClient = this._httpClientFactory.CreateClient(); - using var fileContent = new ByteArrayContent(File.ReadAllBytes(validatedLocalPath)); + using var localFile = SecureFileSystem.OpenRead( + localFilePath, + this._settings.AllowedUploadDirectories!, + out _); + using var fileContent = new StreamContent(localFile); using var multipartFormDataContent = new MultipartFormDataContent() { @@ -181,7 +185,11 @@ public async Task DownloadFileAsync( { try { - File.WriteAllBytes(validatedLocalPath, fileContent); + using var localFile = SecureFileSystem.OpenWrite( + localFilePath!, + this._settings.AllowedDownloadDirectories, + out _); + localFile.Write(fileContent, 0, fileContent.Length); } catch (Exception ex) { @@ -328,11 +336,11 @@ private string ValidateLocalPathForUpload(string localFilePath) "File upload requires 'AllowedUploadDirectories' to be configured."); } - var canonicalPath = PathUtilities.GetSafeFullPath(localFilePath); + var canonicalPath = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(localFilePath)); foreach (var allowedDir in this._settings.AllowedUploadDirectories) { - var canonicalAllowedDir = PathUtilities.GetSafeFullPath(allowedDir); + var canonicalAllowedDir = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(allowedDir)); // Ensure we match the directory correctly by appending separator var separator = Path.DirectorySeparatorChar.ToString(); var allowedDirWithSeparator = canonicalAllowedDir.EndsWith(separator, PathUtilities.PathComparison) @@ -357,13 +365,13 @@ private string ValidateLocalPathForUpload(string localFilePath) /// Thrown when the path is not allowed. private string ValidateLocalPathForDownload(string localFilePath) { - // If no restrictions configured, allow all paths (permissive by default for downloads) if (this._settings.AllowedDownloadDirectories is null || !this._settings.AllowedDownloadDirectories.Any()) { - return PathUtilities.GetSafeFullPath(localFilePath); + throw new InvalidOperationException( + "Saving a download requires 'AllowedDownloadDirectories' to be configured."); } - var canonicalFilePath = PathUtilities.GetSafeFullPath(localFilePath); + var canonicalFilePath = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(localFilePath)); // Get the directory of the resolved target file path. var targetDirectory = Path.GetDirectoryName(canonicalFilePath); @@ -376,7 +384,7 @@ private string ValidateLocalPathForDownload(string localFilePath) foreach (var allowedDir in this._settings.AllowedDownloadDirectories) { - var canonicalAllowedDir = PathUtilities.GetSafeFullPath(allowedDir); + var canonicalAllowedDir = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(allowedDir)); // Ensure we match the directory correctly by appending separator var separator = Path.DirectorySeparatorChar.ToString(); var allowedDirWithSeparator = canonicalAllowedDir.EndsWith(separator, PathUtilities.PathComparison) diff --git a/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonSettings.cs b/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonSettings.cs index b252f410f3ea..5561c9c54199 100644 --- a/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonSettings.cs +++ b/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonSettings.cs @@ -52,8 +52,8 @@ public class SessionsPythonSettings /// /// Gets or sets the list of allowed local directories for file downloads. - /// If configured, files can only be downloaded to these directories. - /// If null or empty, all paths are allowed (permissive by default). + /// Files can only be saved to these directories. If null or empty, saving downloads + /// to disk is denied. Downloads that return bytes without a local path are still allowed. /// [JsonIgnore] public IEnumerable? AllowedDownloadDirectories { get; set; } diff --git a/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs b/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs index 2df1770a633b..2f6116bc7fb1 100644 --- a/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Core/FileIOPlugin.cs @@ -64,7 +64,9 @@ public async Task ReadAsync([Description("Source file")] string path) throw new InvalidOperationException("Reading from the provided location is not allowed."); } - using var reader = File.OpenText(canonicalPath); + using var stream = SecureFileSystem.OpenRead( + canonicalPath, this._allowedFolders, out _, rejectReadOnly: true); + using var reader = new StreamReader(stream); return await reader.ReadToEndAsync().ConfigureAwait(false); } @@ -87,14 +89,9 @@ public async Task WriteAsync( throw new InvalidOperationException("Writing to the provided location is not allowed."); } - if (this.DisableFileOverwrite && File.Exists(canonicalPath)) - { - throw new InvalidOperationException("Overwriting existing files is disabled."); - } - byte[] text = Encoding.UTF8.GetBytes(content); var fileMode = this.DisableFileOverwrite ? FileMode.CreateNew : FileMode.Create; - using var writer = new FileStream(canonicalPath, fileMode, FileAccess.Write, FileShare.None); + using var writer = this.OpenWrite(canonicalPath, fileMode); await writer.WriteAsync(text #if !NET , 0, text.Length @@ -105,6 +102,18 @@ await writer.WriteAsync(text #region private private HashSet? _allowedFolders = []; + private FileStream OpenWrite(string path, FileMode mode) + { + try + { + return SecureFileSystem.OpenWrite(path, this._allowedFolders, out _, mode, FileShare.None); + } + catch (SecureFileSystem.FileAlreadyExistsException ex) + { + throw new InvalidOperationException("Overwriting existing files is disabled.", ex); + } + } + /// /// If a list of allowed folder has been provided, the folder of the provided filePath is checked /// to verify it is in the allowed folder list. Paths are canonicalized before comparison. @@ -171,12 +180,6 @@ private bool TryGetAllowedFilePath(string path, out string canonicalPath) if (canonicalDir.StartsWith(canonicalAllowed, PathUtilities.PathComparison) || (canonicalDir + separator).Equals(canonicalAllowed, PathUtilities.PathComparison)) { - if (File.Exists(canonicalPath) && File.GetAttributes(canonicalPath).HasFlag(FileAttributes.ReadOnly)) - { - // Most environments will throw this with OpenWrite, but running inside docker on Linux will not. - throw new UnauthorizedAccessException("File is read-only."); - } - return true; } } diff --git a/dotnet/src/Plugins/Plugins.Core/Plugins.Core.csproj b/dotnet/src/Plugins/Plugins.Core/Plugins.Core.csproj index 9b0f06f623da..2cff84d8ef8d 100644 --- a/dotnet/src/Plugins/Plugins.Core/Plugins.Core.csproj +++ b/dotnet/src/Plugins/Plugins.Core/Plugins.Core.csproj @@ -25,6 +25,10 @@ + + + + diff --git a/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs b/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs index d95067588cb8..46522b0e0338 100644 --- a/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs @@ -96,7 +96,8 @@ public async Task ReadTextAsync( throw new InvalidOperationException("Reading from the provided location is not allowed."); } - using var stream = await this._fileSystemConnector.GetFileContentStreamAsync(canonicalPath, cancellationToken).ConfigureAwait(false); + using var stream = await this._fileSystemConnector.GetFileContentStreamAsync( + canonicalPath, this._allowedDirectories!, cancellationToken).ConfigureAwait(false); return this._documentConnector.ReadText(stream); } @@ -116,22 +117,17 @@ public async Task AppendTextAsync( 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(canonicalPath, 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 + var opened = await this._fileSystemConnector.OpenUpdateAsync( + canonicalPath, this._allowedDirectories!, cancellationToken).ConfigureAwait(false); + using var stream = opened.Stream; + if (opened.Created) { 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}", canonicalPath); - this._documentConnector.AppendText(stream, text); } + + this._logger.LogDebug("Writing text to {0}", canonicalPath); + this._documentConnector.AppendText(stream, text); } #region private @@ -139,51 +135,14 @@ public async Task AppendTextAsync( /// /// Expands environment variables and resolves the path to its canonical form. - /// This must be called before validation to prevent validate/use mismatches. + /// The connector must still enforce the allowed directories when opening the file. /// private static string CanonicalizePath(string path) { - Verify.NotNullOrWhiteSpace(path); - - if (IsUncOrExtendedPath(path)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(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 (IsUncOrExtendedPath(expanded)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); - } - - // Resolve the full path first (a pure string operation that does not touch the - // filesystem). A relative path can still resolve to a UNC path here, for example - // when the current directory is a UNC share, so re-check before GetSafeFullPath - // probes the filesystem while resolving symbolic links. - var fullPath = Path.GetFullPath(expanded); - if (IsUncOrExtendedPath(fullPath)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); - } - + var fullPath = SecureFileSystem.NormalizePath(path, expandEnvironmentVariables: true); return PathUtilities.GetSafeFullPath(fullPath); } - /// - /// Returns true if the path starts with any combination of two directory separators - /// (e.g. \\, //, /\, \/), all of which resolve to UNC or - /// extended-length/device paths on Windows. - /// - private static bool IsUncOrExtendedPath(string path) => - path.Length >= 2 && - (path[0] is '/' or '\\') && - (path[1] is '/' or '\\'); - /// /// Checks whether a canonicalized file path falls within one of the allowed directories. /// Subdirectories of allowed directories are also permitted. diff --git a/dotnet/src/Plugins/Plugins.Document/FileSystem/IFileSystemConnector.cs b/dotnet/src/Plugins/Plugins.Document/FileSystem/IFileSystemConnector.cs index ce4a0d88856b..29af80238b18 100644 --- a/dotnet/src/Plugins/Plugins.Document/FileSystem/IFileSystemConnector.cs +++ b/dotnet/src/Plugins/Plugins.Document/FileSystem/IFileSystemConnector.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. +using System.Collections.Generic; using System.IO; using System.Threading; using System.Threading.Tasks; @@ -15,28 +16,21 @@ public interface IFileSystemConnector /// Get the contents of a file as a read-only stream. /// /// Path to the file. + /// Nonempty list of trusted directories. The connector must + /// enforce this restriction while opening the file, not just check a path before opening it. /// The to monitor for cancellation requests. The default is . - Task GetFileContentStreamAsync(string filePath, CancellationToken cancellationToken = default); + Task GetFileContentStreamAsync( + string filePath, IEnumerable allowedDirectories, CancellationToken cancellationToken = default); /// - /// Get a writeable stream to an existing file. + /// Securely open or create a document without truncating existing content. /// /// Path to file. + /// Nonempty list of trusted directories. Opening must not + /// follow a replacement link outside these directories. /// The to monitor for cancellation requests. The default is . - Task GetWriteableFileStreamAsync(string filePath, CancellationToken cancellationToken = default); - - /// - /// Create a new file and get a writeable stream to it. - /// - /// Path to file. - /// The to monitor for cancellation requests. The default is . - Task CreateFileAsync(string filePath, CancellationToken cancellationToken = default); - - /// - /// Determine whether a file exists at the specified path. - /// - /// Path to file. - /// The to monitor for cancellation requests. The default is . - /// True if file exists, false otherwise. - Task FileExistsAsync(string filePath, CancellationToken cancellationToken = default); + /// A caller-owned, seekable read/write stream and whether this operation created + /// the file. Creation must be determined by the open itself, not a separate existence check. + Task<(Stream Stream, bool Created)> OpenUpdateAsync( + string filePath, IEnumerable allowedDirectories, CancellationToken cancellationToken = default); } diff --git a/dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs b/dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs index 2fef0ae8db2d..cf9a14179a81 100644 --- a/dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs +++ b/dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs @@ -1,105 +1,57 @@ // Copyright (c) Microsoft. All rights reserved. using System; +using System.Collections.Generic; using System.IO; using System.Threading; using System.Threading.Tasks; namespace Microsoft.SemanticKernel.Plugins.Document.FileSystem; -#pragma warning disable CA1031 // Exceptions are caught and returned in a task +#pragma warning disable CA1031 // Exceptions are returned in the task, matching the connector contract. /// -/// Connector for local filesystem +/// Connector for local filesystem access restricted to trusted directories. /// public class LocalFileSystemConnector : IFileSystemConnector { - /// - /// Get the contents of a file as a read-only stream. - /// - /// Path to file - /// The to monitor for cancellation requests. The default is . - /// - /// - /// - /// - /// - /// - /// - /// - /// - public Task GetFileContentStreamAsync(string filePath, CancellationToken cancellationToken = default) + /// + public Task GetFileContentStreamAsync( + string filePath, IEnumerable allowedDirectories, CancellationToken cancellationToken = default) { try { - return Task.FromResult(File.Open(filePath, FileMode.Open, FileAccess.Read)); - } - catch (Exception e) - { - return Task.FromException(e); + cancellationToken.ThrowIfCancellationRequested(); + return Task.FromResult(SecureFileSystem.OpenRead( + filePath, allowedDirectories, out _, share: FileShare.None)); } - } - - /// - /// Get a writeable stream to a file. - /// - /// Path to file - /// The to monitor for cancellation requests. The default is . - /// - /// - /// - /// - /// - /// - /// - /// - /// - public Task GetWriteableFileStreamAsync(string filePath, CancellationToken cancellationToken = default) - { - try + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - return Task.FromResult(File.Open(filePath, FileMode.Open, FileAccess.ReadWrite)); + return Task.FromCanceled(cancellationToken); } - catch (Exception e) + catch (Exception ex) { - return Task.FromException(e); + return Task.FromException(ex); } } - /// - /// Get a writeable stream to a file. - /// - /// Path to file - /// The to monitor for cancellation requests. The default is . - /// - /// - /// - /// - /// - /// - /// - public Task CreateFileAsync(string filePath, CancellationToken cancellationToken = default) + /// + public Task<(Stream Stream, bool Created)> OpenUpdateAsync( + string filePath, IEnumerable allowedDirectories, CancellationToken cancellationToken = default) { try { - return Task.FromResult(File.Create(filePath)); + cancellationToken.ThrowIfCancellationRequested(); + Stream stream = SecureFileSystem.OpenUpdate(filePath, allowedDirectories, out var created); + return Task.FromResult((stream, created)); } - catch (Exception e) - { - return Task.FromException(e); - } - } - - /// - public Task FileExistsAsync(string filePath, CancellationToken cancellationToken = default) - { - try + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) { - return Task.FromResult(File.Exists(filePath)); + return Task.FromCanceled<(Stream, bool)>(cancellationToken); } - catch (Exception e) + catch (Exception ex) { - return Task.FromException(e); + return Task.FromException<(Stream, bool)>(ex); } } } diff --git a/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs b/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs index 35e044cd9d7d..53e78378a1e7 100644 --- a/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs +++ b/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs @@ -185,8 +185,10 @@ public async Task UploadFileAsync( this._logger.LogDebug("Uploading file '{0}'", canonicalPath); + using var content = SecureFileSystem.OpenRead(canonicalPath, this._allowedUploadDirectories, out _); + // TODO Add support for large file uploads (i.e. upload sessions) - await this._connector.UploadSmallFileAsync(canonicalPath, destinationPath, cancellationToken).ConfigureAwait(false); + await this._connector.UploadSmallFileAsync(content, destinationPath, cancellationToken).ConfigureAwait(false); } /// @@ -292,49 +294,14 @@ private static string NormalizeRemotePath(string path) /// /// Expands environment variables and resolves the path to its canonical form. - /// This must be called before validation to prevent validate/use mismatches. + /// The upload still uses a secure open because this path can change after validation. /// private static string CanonicalizePath(string path) { - Ensure.NotNullOrWhitespace(path, nameof(path)); - - if (IsUncOrExtendedPath(path)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(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 (IsUncOrExtendedPath(expanded)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); - } - - // Re-check after canonicalization: a relative path can still resolve to a UNC path, - // for example when the current directory is a UNC share. - var fullPath = Path.GetFullPath(expanded); - if (IsUncOrExtendedPath(fullPath)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); - } - - return fullPath; + var fullPath = SecureFileSystem.NormalizePath(path, expandEnvironmentVariables: true); + return PathUtilities.GetSafeFullPath(fullPath); } - /// - /// Returns true if the path starts with any combination of two directory separators - /// (e.g. \\, //, /\, \/), all of which resolve to UNC or - /// extended-length/device paths on Windows. - /// - private static bool IsUncOrExtendedPath(string path) => - path.Length >= 2 && - (path[0] is '/' or '\\') && - (path[1] is '/' or '\\'); - /// /// Checks whether a canonicalized file path falls within one of the allowed upload directories. /// Subdirectories of allowed directories are also permitted. @@ -357,7 +324,7 @@ private bool IsUploadPathAllowed(string canonicalPath) foreach (var allowedDirectory in this._allowedUploadDirectories) { - var canonicalAllowed = Path.GetFullPath(allowedDirectory); + var canonicalAllowed = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(allowedDirectory)); var separator = Path.DirectorySeparatorChar.ToString(); if (!canonicalAllowed.EndsWith(separator, s_pathComparison)) { diff --git a/dotnet/src/Plugins/Plugins.MsGraph/Connectors/OneDriveConnector.cs b/dotnet/src/Plugins/Plugins.MsGraph/Connectors/OneDriveConnector.cs index 262e7453944e..fa102cd775c8 100644 --- a/dotnet/src/Plugins/Plugins.MsGraph/Connectors/OneDriveConnector.cs +++ b/dotnet/src/Plugins/Plugins.MsGraph/Connectors/OneDriveConnector.cs @@ -74,21 +74,25 @@ await this._graphServiceClient } /// - public async Task UploadSmallFileAsync(string filePath, string destinationPath, CancellationToken cancellationToken = default) + public async Task UploadSmallFileAsync(Stream content, string destinationPath, CancellationToken cancellationToken = default) { - Ensure.NotNullOrWhitespace(filePath, nameof(filePath)); + Verify.NotNull(content); Ensure.NotNullOrWhitespace(destinationPath, nameof(destinationPath)); - filePath = Environment.ExpandEnvironmentVariables(filePath); + if (!content.CanRead || !content.CanSeek) + { + throw new ArgumentException("Upload content must be readable and seekable.", nameof(content)); + } - long fileSize = new FileInfo(filePath).Length; + long fileSize = content.Length - content.Position; if (fileSize > 4 * 1024 * 1024) { throw new IOException("File is too large to upload - function currently only supports files up to 4MB."); } - using FileStream fileContentStream = new(filePath, FileMode.Open, FileAccess.Read); - + // Graph disposes its request body. Borrow the stream so the caller still owns + // the securely opened file, including when a request fails. + using var uploadStream = new BorrowedReadStream(content); DriveItem? response = null; try @@ -97,7 +101,7 @@ public async Task UploadSmallFileAsync(string filePath, string destinationPath, response = await this._graphServiceClient .Drives[myDrive!.Id].Root - .ItemWithPath(destinationPath).Content.PutAsync(fileContentStream, cancellationToken: cancellationToken).ConfigureAwait(false); + .ItemWithPath(destinationPath).Content.PutAsync(uploadStream, cancellationToken: cancellationToken).ConfigureAwait(false); } catch (ServiceException ex) { @@ -113,6 +117,41 @@ public async Task UploadSmallFileAsync(string filePath, string destinationPath, } } + /// + /// Lets Graph read the upload without taking ownership of the caller's stream. + /// + /// + /// Graph closes its request-body stream. Closing this wrapper leaves the underlying + /// stream open, so the caller remains responsible for closing the securely opened + /// file whether the upload succeeds or fails. Reads and seeks use that same stream; + /// the wrapper neither copies the file nor reopens its path. + /// + private sealed class BorrowedReadStream(Stream source) : Stream + { + public override bool CanRead => source.CanRead; + public override bool CanSeek => source.CanSeek; + public override bool CanWrite => false; + public override long Length => source.Length; + public override long Position + { + get => source.Position; + set => source.Position = value; + } + + public override void Flush() => source.Flush(); + + public override int Read(byte[] buffer, int offset, int count) => source.Read(buffer, offset, count); + + public override Task ReadAsync(byte[] buffer, int offset, int count, CancellationToken cancellationToken) => + source.ReadAsync(buffer, offset, count, cancellationToken); + + public override long Seek(long offset, SeekOrigin origin) => source.Seek(offset, origin); + + public override void SetLength(long value) => throw new NotSupportedException(); + + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + } + /// public async Task CreateShareLinkAsync(string filePath, string type = "view", string scope = "organization", CancellationToken cancellationToken = default) diff --git a/dotnet/src/Plugins/Plugins.MsGraph/ICloudDriveConnector.cs b/dotnet/src/Plugins/Plugins.MsGraph/ICloudDriveConnector.cs index e603ff68cca7..60e91c44f296 100644 --- a/dotnet/src/Plugins/Plugins.MsGraph/ICloudDriveConnector.cs +++ b/dotnet/src/Plugins/Plugins.MsGraph/ICloudDriveConnector.cs @@ -31,8 +31,9 @@ public interface ICloudDriveConnector /// /// Upload a small file (less than 4MB). /// - /// Path of the local file to upload. + /// Readable, seekable content stream positioned at the start of the upload. + /// The caller owns the stream and must keep it open until the upload completes. /// Remote path to store the file, which is relative to the root of the OneDrive folder and should begin with the '/' character. /// The to monitor for cancellation requests. The default is . - Task UploadSmallFileAsync(string filePath, string destinationPath, CancellationToken cancellationToken = default); + Task UploadSmallFileAsync(Stream content, string destinationPath, CancellationToken cancellationToken = default); } diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs new file mode 100644 index 000000000000..7b8dd353ea4e --- /dev/null +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs @@ -0,0 +1,427 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.Collections.Generic; +using System.IO; +using System.Runtime.InteropServices; +using System.Text; +using System.Threading.Tasks; +using Microsoft.SemanticKernel; +using Xunit; + +namespace SemanticKernel.Plugins.UnitTests.Core; + +public sealed class SecureFileSystemTests : IDisposable +{ + private readonly string _tempDirectory; + private readonly string _allowedDirectory; + private readonly string _outsideDirectory; + + public SecureFileSystemTests() + { + this._tempDirectory = Path.Combine( + Path.GetTempPath(), + $"SecureFileSystemTests_{Guid.NewGuid():N}"); + this._allowedDirectory = Path.Combine(this._tempDirectory, "allowed"); + this._outsideDirectory = Path.Combine(this._tempDirectory, "outside"); + + Directory.CreateDirectory(this._allowedDirectory); + Directory.CreateDirectory(this._outsideDirectory); + } + + [Fact] + public void OpenReadReadsFileInsideAllowedDirectory() + { + // Arrange + var filePath = Path.Combine(this._allowedDirectory, "input.txt"); + File.WriteAllText(filePath, "allowed content"); + + // Act + using var stream = SecureFileSystem.OpenRead( + filePath, + [this._allowedDirectory], + out var resolvedPath); + using var reader = new StreamReader(stream); + + // Assert + Assert.Equal("allowed content", reader.ReadToEnd()); + Assert.Equal(PathUtilities.GetSafeFullPath(filePath), resolvedPath); + } + + [Fact] + public void OpenReadRejectsFileOutsideAllowedDirectory() + { + // Arrange + var filePath = Path.Combine(this._outsideDirectory, "outside.txt"); + File.WriteAllText(filePath, "outside content"); + + // Act and assert + var exception = Assert.Throws( + () => SecureFileSystem.OpenRead(filePath, [this._allowedDirectory], out _)); + + Assert.Contains("not within allowed directories", exception.Message); + } + + [SymbolicLinkFact] + public void OpenReadRejectsSymbolicLinkToFileOutsideAllowedDirectory() + { + // Arrange + var outsideFile = Path.Combine(this._outsideDirectory, "outside.txt"); + var linkPath = Path.Combine(this._allowedDirectory, "link.txt"); + File.WriteAllText(outsideFile, "outside content"); + + File.CreateSymbolicLink(linkPath, outsideFile); + + // Act and assert + Assert.Throws( + () => SecureFileSystem.OpenRead(linkPath, [this._allowedDirectory], out _)); + } + + [SymbolicLinkFact] + public void OpenReadRejectsSymbolicLinkDirectoryToFileOutsideAllowedDirectory() + { + // Arrange + var outsideFile = Path.Combine(this._outsideDirectory, "outside.txt"); + var linkDirectory = Path.Combine(this._allowedDirectory, "linked-directory"); + File.WriteAllText(outsideFile, "outside content"); + + Directory.CreateSymbolicLink(linkDirectory, this._outsideDirectory); + + // Act and assert + Assert.Throws( + () => SecureFileSystem.OpenRead( + Path.Combine(linkDirectory, "outside.txt"), + [this._allowedDirectory], + out _)); + } + + [Fact] + public void OpenWriteCreatesFileInsideAllowedDirectory() + { + // Arrange + var filePath = Path.Combine(this._allowedDirectory, "new.txt"); + var content = Encoding.UTF8.GetBytes("new content"); + + // Act + using (var stream = SecureFileSystem.OpenWrite( + filePath, + [this._allowedDirectory], + out var resolvedPath)) + { + stream.Write(content, 0, content.Length); + Assert.Equal(PathUtilities.GetSafeFullPath(filePath), resolvedPath); + } + + // Assert + Assert.Equal("new content", File.ReadAllText(filePath)); + } + + [Fact] + public void OpenWriteTruncatesExistingFileInsideAllowedDirectory() + { + // Arrange + var filePath = Path.Combine(this._allowedDirectory, "existing.txt"); + var content = Encoding.UTF8.GetBytes("short"); + File.WriteAllText(filePath, "existing longer content"); + + // Act + using (var stream = SecureFileSystem.OpenWrite( + filePath, + [this._allowedDirectory], + out _)) + { + stream.Write(content, 0, content.Length); + } + + // Assert + Assert.Equal("short", File.ReadAllText(filePath)); + } + + [SymbolicLinkFact] + public void OpenWriteRejectsSymbolicLinkToFileOutsideAllowedDirectory() + { + // Arrange + var outsideFile = Path.Combine(this._outsideDirectory, "outside.txt"); + var linkPath = Path.Combine(this._allowedDirectory, "link.txt"); + File.WriteAllText(outsideFile, "must not change"); + + File.CreateSymbolicLink(linkPath, outsideFile); + + // Act + Assert.Throws( + () => SecureFileSystem.OpenWrite(linkPath, [this._allowedDirectory], out _)); + + // Assert + Assert.Equal("must not change", File.ReadAllText(outsideFile)); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void EmptyAllowedDirectoriesDenyReadsAndWrites(bool useNull) + { + var path = Path.Combine(this._allowedDirectory, "denied.txt"); + string[]? allowed = useNull ? null : []; + + Assert.Throws(() => SecureFileSystem.OpenRead(path, allowed, out _)); + Assert.Throws(() => SecureFileSystem.OpenWrite(path, allowed, out _)); + Assert.Throws(() => SecureFileSystem.OpenUpdate(path, allowed, out _)); + Assert.False(File.Exists(path)); + } + + [Theory] + [InlineData(@"\\server\share\file.txt")] + [InlineData("//server/share/file.txt")] + [InlineData(@"/\server\share\file.txt")] + [InlineData(@"\/server/share/file.txt")] + [InlineData(@"\\?\C:\file.txt")] + [InlineData(@"\\.\device")] + public void NetworkAndDevicePathsAreRejected(string path) + { + Assert.Throws(() => SecureFileSystem.OpenRead(path, [this._allowedDirectory], out _)); + Assert.Throws(() => SecureFileSystem.OpenWrite(path, [this._allowedDirectory], out _)); + } + + [Fact] + public void RootDirectoryCanBeAllowed() + { + var path = Path.Combine(this._allowedDirectory, "root.txt"); + File.WriteAllText(path, "content"); + + using var stream = SecureFileSystem.OpenRead(path, [Path.GetPathRoot(path)!], out _); + using var reader = new StreamReader(stream); + Assert.Equal("content", reader.ReadToEnd()); + } + + [Fact] + public void DirectoryWithMatchingNamePrefixIsNotAllowed() + { + var directory = this._allowedDirectory + "-outside"; + Directory.CreateDirectory(directory); + var path = Path.Combine(directory, "file.txt"); + File.WriteAllText(path, "unchanged"); + + Assert.Throws(() => SecureFileSystem.OpenRead(path, [this._allowedDirectory], out _)); + Assert.Throws(() => SecureFileSystem.OpenWrite(path, [this._allowedDirectory], out _)); + Assert.Equal("unchanged", File.ReadAllText(path)); + } + + [Fact] + public void CreateNewDoesNotOverwriteExistingFile() + { + var path = Path.Combine(this._allowedDirectory, "existing.txt"); + File.WriteAllText(path, "unchanged"); + + Assert.Throws( + () => SecureFileSystem.OpenWrite(path, [this._allowedDirectory], out _, FileMode.CreateNew)); + Assert.Equal("unchanged", File.ReadAllText(path)); + } + + [Fact] + public void CreateNewCreatesMissingFile() + { + var path = Path.Combine(this._allowedDirectory, "new.txt"); + using (var stream = SecureFileSystem.OpenWrite(path, [this._allowedDirectory], out _, FileMode.CreateNew)) + { + stream.WriteByte(42); + } + + Assert.Equal(new byte[] { 42 }, File.ReadAllBytes(path)); + } + + [Fact] + public void OpenUpdateReportsCreationWithoutTruncatingExistingFiles() + { + var path = Path.Combine(this._allowedDirectory, "document.txt"); + using (var stream = SecureFileSystem.OpenUpdate(path, [this._allowedDirectory], out var created)) + { + Assert.True(created); + Assert.True(stream.CanRead); + Assert.True(stream.CanWrite); + Assert.True(stream.CanSeek); + stream.WriteByte(42); + } + + using (var stream = SecureFileSystem.OpenUpdate(path, [this._allowedDirectory], out var created)) + { + Assert.False(created); + Assert.Equal(42, stream.ReadByte()); + } + } + + [Fact] + public void OpenUpdateDoesNotTreatAnExistingEmptyFileAsNew() + { + var path = Path.Combine(this._allowedDirectory, "empty.txt"); + File.WriteAllText(path, string.Empty); + using var stream = SecureFileSystem.OpenUpdate(path, [this._allowedDirectory], out var created); + Assert.False(created); + } + + [Fact] + public void WritesToReadOnlyFilesAreRejectedBeforeTruncation() + { + var path = Path.Combine(this._allowedDirectory, "readonly.txt"); + File.WriteAllText(path, "unchanged"); + File.SetAttributes(path, FileAttributes.ReadOnly); + try + { + Assert.Throws( + () => SecureFileSystem.OpenWrite(path, [this._allowedDirectory], out _)); + Assert.Throws( + () => SecureFileSystem.OpenUpdate(path, [this._allowedDirectory], out _)); + Assert.Equal("unchanged", File.ReadAllText(path)); + } + finally + { + File.SetAttributes(path, FileAttributes.Normal); + } + } + + [SymbolicLinkTheory] + [InlineData(false, false)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(true, true)] + public void LinkReplacementAfterCanonicalizationIsRejected(bool write, bool replaceParent) + { + var parent = Path.Combine(this._allowedDirectory, "parent"); + Directory.CreateDirectory(parent); + var path = Path.Combine(parent, "file.txt"); + var outside = Path.Combine(this._outsideDirectory, "file.txt"); + File.WriteAllText(path, "allowed"); + File.WriteAllText(outside, "outside"); + + IEnumerable AllowedDirectories() + { + yield return this._outsideDirectory; + // The first comparison has already resolved the caller's path. Replace a + // component before returning the allowed folder used by the native open. + if (replaceParent) + { + Directory.Move(parent, parent + "-original"); + Directory.CreateSymbolicLink(parent, this._outsideDirectory); + } + else + { + File.Move(path, path + ".original"); + File.CreateSymbolicLink(path, outside); + } + + yield return this._allowedDirectory; + } + + if (write) + { + Assert.Throws( + () => SecureFileSystem.OpenWrite(path, AllowedDirectories(), out _)); + } + else + { + Assert.Throws( + () => SecureFileSystem.OpenRead(path, AllowedDirectories(), out _)); + } + + Assert.Equal("outside", File.ReadAllText(outside)); + var original = replaceParent ? Path.Combine(parent + "-original", "file.txt") : path + ".original"; + Assert.Equal("allowed", File.ReadAllText(original)); + } + + [SymbolicLinkFact] + public void ExistingLinkToAllowedFileCanBeRead() + { + var target = Path.Combine(this._allowedDirectory, "target.txt"); + var link = Path.Combine(this._allowedDirectory, "link.txt"); + File.WriteAllText(target, "allowed"); + File.CreateSymbolicLink(link, target); + using var stream = SecureFileSystem.OpenRead(link, [this._allowedDirectory], out _); + using var reader = new StreamReader(stream); + Assert.Equal("allowed", reader.ReadToEnd()); + } + + [SymbolicLinkTheory] + [InlineData(false)] + [InlineData(true)] + public void OpenStreamCannotBeRedirectedByReplacingItsName(bool write) + { + var path = Path.Combine(this._allowedDirectory, "file.txt"); + var original = path + ".original"; + var outside = Path.Combine(this._outsideDirectory, "file.txt"); + File.WriteAllText(path, "allowed"); + File.WriteAllText(outside, "outside"); + using (var stream = write + ? SecureFileSystem.OpenWrite(path, [this._allowedDirectory], out _) + : SecureFileSystem.OpenRead(path, [this._allowedDirectory], out _)) + { + if (OperatingSystem.IsWindows()) + { + Assert.Throws(() => File.Move(path, original)); + } + else + { + File.Move(path, original); + File.CreateSymbolicLink(path, outside); + } + + if (write) + { + stream.WriteByte(42); + } + else + { + using var reader = new StreamReader(stream, leaveOpen: true); + Assert.Equal("allowed", reader.ReadToEnd()); + } + } + + Assert.Equal("outside", File.ReadAllText(outside)); + if (write) + { + Assert.Equal(new byte[] { 42 }, File.ReadAllBytes(OperatingSystem.IsWindows() ? path : original)); + } + } + + [Fact] + public void MissingParentIsNotCreated() + { + var path = Path.Combine(this._allowedDirectory, "missing", "file.txt"); + Assert.Throws(() => SecureFileSystem.OpenWrite(path, [this._allowedDirectory], out _)); + Assert.False(Directory.Exists(Path.GetDirectoryName(path))); + } + + [UnixFact(Timeout = 5000)] + public async Task NamedPipeIsRejectedWithoutWaitingForAnotherProcessAsync() + { + var path = Path.Combine(this._allowedDirectory, "pipe"); + Assert.Equal(0, MakeFifo(path, 0x180)); + await Task.Run(() => + { + Assert.Throws(() => SecureFileSystem.OpenRead(path, [this._allowedDirectory], out _)); + Assert.Throws(() => SecureFileSystem.OpenWrite(path, [this._allowedDirectory], out _)); + }); + } + + [UnixFact] + public void DeviceIsNotTreatedAsAnOrdinaryFile() + { + Assert.Throws(() => SecureFileSystem.OpenRead("/dev/null", ["/dev"], out _)); + Assert.Throws(() => SecureFileSystem.OpenWrite("/dev/null", ["/dev"], out _)); + } + + [DllImport("libc", EntryPoint = "mkfifo", SetLastError = true)] + private static extern int MakeFifo(string path, uint mode); + + public void Dispose() + { + try + { + Directory.Delete(this._tempDirectory, recursive: true); + } + catch (IOException) + { + } + catch (UnauthorizedAccessException) + { + } + } +} diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs index ce262555f183..c633765f5d0d 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs @@ -368,8 +368,7 @@ public async Task ItShouldDownloadFileSavingInDiskAsync() Content = new ByteArrayContent(responseContent), }; - // Downloads are permissive by default - no need for special settings - var plugin = new SessionsPythonPlugin(this._defaultSettings, this._httpClientFactory); + var plugin = new SessionsPythonPlugin(this._settingsWithFileOperationsEnabled, this._httpClientFactory); // Act var result = await plugin.DownloadFileAsync("test.txt", downloadDiskPath); @@ -380,6 +379,21 @@ public async Task ItShouldDownloadFileSavingInDiskAsync() Assert.Equal(responseContent, await File.ReadAllBytesAsync(downloadDiskPath)); } + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task ItShouldDenyLocalDownloadsWithoutAllowedDirectoriesAsync(bool useNull) + { + this._defaultSettings.AllowedDownloadDirectories = useNull ? null : []; + var plugin = new SessionsPythonPlugin(this._defaultSettings, this._httpClientFactory); + + var exception = await Assert.ThrowsAsync( + () => plugin.DownloadFileAsync("test.txt", Path.Combine(Path.GetTempPath(), "denied.txt"))); + + Assert.Contains("AllowedDownloadDirectories", exception.Message); + Assert.Null(this._messageHandlerStub.RequestUri); + } + /// /// Test the allowed domains for the endpoint. /// @@ -624,6 +638,66 @@ public async Task ItShouldDenyDownloadThroughSymlinkOutsideAllowedDirectoriesAsy } } + [Fact] + public async Task ItShouldDenyDownloadWhenFileIsReplacedBySymlinkAfterValidationAsync() + { + // Arrange + var tempDir = Path.Combine(Path.GetTempPath(), $"SessionsPythonPluginTests_{Guid.NewGuid():N}"); + var allowedDir = Path.Combine(tempDir, "allowed"); + var outsideDir = Path.Combine(tempDir, "outside"); + Directory.CreateDirectory(allowedDir); + Directory.CreateDirectory(outsideDir); + + try + { + var downloadPath = Path.Combine(allowedDir, "download.txt"); + var outsideFile = Path.Combine(outsideDir, "outside.txt"); + await File.WriteAllTextAsync(downloadPath, "safe"); + await File.WriteAllTextAsync(outsideFile, "outside"); + + using var messageHandler = new BlockingHttpMessageHandler(new byte[] { 1, 2, 3 }); + using var httpClient = new HttpClient(messageHandler, disposeHandler: false); + var httpClientFactoryMock = new Mock(); + httpClientFactoryMock.Setup(f => f.CreateClient(It.IsAny())).Returns(httpClient); + + var settings = new SessionsPythonSettings( + sessionId: Guid.NewGuid().ToString(), + endpoint: new Uri("http://localhost:8888")) + { + AllowedDomains = ["localhost"], + AllowedDownloadDirectories = new[] { allowedDir } + }; + + var plugin = new SessionsPythonPlugin(settings, httpClientFactoryMock.Object); + var downloadTask = plugin.DownloadFileAsync("test.txt", downloadPath); + await messageHandler.RequestReceived; + + File.Delete(downloadPath); + try + { + File.CreateSymbolicLink(downloadPath, outsideFile); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + messageHandler.ReleaseResponse(); + await downloadTask; + return; + } + + // Act + messageHandler.ReleaseResponse(); + var exception = await Assert.ThrowsAsync(() => downloadTask); + + // Assert + Assert.Contains("Failed to write file to disk", exception.Message); + Assert.Equal("outside", await File.ReadAllTextAsync(outsideFile)); + } + finally + { + TryDeleteDirectory(tempDir); + } + } + [Fact] public async Task ItShouldAllowDownloadDirectlyInsideAllowedDirectoryAsync() { @@ -753,4 +827,29 @@ private static void TryDeleteDirectory(string path) { } } + + private sealed class BlockingHttpMessageHandler(byte[] responseContent) : HttpMessageHandler + { + private readonly TaskCompletionSource _requestReceived = + new(TaskCreationOptions.RunContinuationsAsynchronously); + private readonly TaskCompletionSource _releaseResponse = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + internal Task RequestReceived => this._requestReceived.Task; + + internal void ReleaseResponse() => this._releaseResponse.TrySetResult(); + + protected override async Task SendAsync( + HttpRequestMessage request, + CancellationToken cancellationToken) + { + this._requestReceived.TrySetResult(); + await this._releaseResponse.Task.WaitAsync(cancellationToken); + + return new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new ByteArrayContent(responseContent), + }; + } + } } diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs index bb982ae39182..17f949af60cf 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft. All rights reserved. using System; +using System.Collections.Generic; using System.IO; using System.Threading; using System.Threading.Tasks; @@ -24,6 +25,7 @@ public async Task ReadTextAsyncSucceedsAsync() var fileSystemConnectorMock = new Mock(); fileSystemConnectorMock .Setup(mock => mock.GetFileContentStreamAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), + It.IsAny>(), It.IsAny())) .ReturnsAsync(Stream.Null); @@ -56,13 +58,10 @@ public async Task AppendTextAsyncFileExistsSucceedsAsync() var fileSystemConnectorMock = new Mock(); fileSystemConnectorMock - .Setup(mock => mock.FileExistsAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), + .Setup(mock => mock.OpenUpdateAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), + It.IsAny>(), It.IsAny())) - .ReturnsAsync(true); - fileSystemConnectorMock - .Setup(mock => mock.GetWriteableFileStreamAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), - It.IsAny())) - .ReturnsAsync(Stream.Null); + .ReturnsAsync((Stream.Null, false)); var documentConnectorMock = new Mock(); documentConnectorMock @@ -91,13 +90,10 @@ public async Task AppendTextAsyncFileDoesNotExistSucceedsAsync() var fileSystemConnectorMock = new Mock(); fileSystemConnectorMock - .Setup(mock => mock.FileExistsAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), + .Setup(mock => mock.OpenUpdateAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), + It.IsAny>(), It.IsAny())) - .ReturnsAsync(false); - fileSystemConnectorMock - .Setup(mock => mock.CreateFileAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), - It.IsAny())) - .ReturnsAsync(Stream.Null); + .ReturnsAsync((Stream.Null, true)); var documentConnectorMock = new Mock(); documentConnectorMock @@ -134,7 +130,8 @@ await Assert.ThrowsAnyAsync(() => target.AppendTextAsync(anyText, null!)); // Assert - fileSystemConnectorMock.Verify(mock => mock.GetWriteableFileStreamAsync(It.IsAny(), It.IsAny()), Times.Never()); + fileSystemConnectorMock.Verify(mock => mock.OpenUpdateAsync( + It.IsAny(), It.IsAny>(), It.IsAny()), Times.Never()); documentConnectorMock.Verify(mock => mock.AppendText(It.IsAny(), It.IsAny()), Times.Never()); } @@ -292,7 +289,8 @@ public async Task ItAllowsSubdirectoriesOfAllowedFoldersAsync() var fileSystemConnectorMock = new Mock(); fileSystemConnectorMock - .Setup(mock => mock.GetFileContentStreamAsync(It.IsAny(), It.IsAny())) + .Setup(mock => mock.GetFileContentStreamAsync( + It.IsAny(), It.IsAny>(), It.IsAny())) .ReturnsAsync(Stream.Null); var documentConnectorMock = new Mock(); diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Document/LocalFileSystemConnectorTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Document/LocalFileSystemConnectorTests.cs new file mode 100644 index 000000000000..d6b30770dede --- /dev/null +++ b/dotnet/src/Plugins/Plugins.UnitTests/Document/LocalFileSystemConnectorTests.cs @@ -0,0 +1,87 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.IO; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.SemanticKernel.Plugins.Document; +using Microsoft.SemanticKernel.Plugins.Document.FileSystem; +using Microsoft.SemanticKernel.Plugins.Document.OpenXml; +using Xunit; + +namespace SemanticKernel.Plugins.UnitTests.Document; + +public sealed class LocalFileSystemConnectorTests : IDisposable +{ + private readonly string _directory = Path.Combine(Path.GetTempPath(), $"LocalFileSystemConnectorTests_{Guid.NewGuid():N}"); + + public LocalFileSystemConnectorTests() + { + Directory.CreateDirectory(this._directory); + } + + [Fact] + public async Task DocumentCanBeCreatedAppendedAndReadWithoutLosingExistingTextAsync() + { + var path = Path.Combine(this._directory, "document.docx"); + var plugin = new DocumentPlugin(new WordDocumentConnector(), new LocalFileSystemConnector()) + { + AllowedDirectories = [this._directory] + }; + + await plugin.AppendTextAsync("first paragraph", path); + await plugin.AppendTextAsync("second paragraph", path); + var text = await plugin.ReadTextAsync(path); + + Assert.Contains("first paragraph", text); + Assert.Contains("second paragraph", text); + } + + [Fact] + public async Task OpenUpdatePreservesAnExistingEmptyFileAsync() + { + var path = Path.Combine(this._directory, "empty.docx"); + File.WriteAllText(path, string.Empty); + var connector = new LocalFileSystemConnector(); + + var opened = await connector.OpenUpdateAsync(path, [this._directory]); + using var stream = opened.Stream; + Assert.False(opened.Created); + Assert.Equal(0, stream.Length); + } + + [Fact] + public async Task CancelledOpenDoesNotCreateFileAsync() + { + var path = Path.Combine(this._directory, "cancelled.docx"); + var connector = new LocalFileSystemConnector(); + using var cancellation = new CancellationTokenSource(); + cancellation.Cancel(); + + await Assert.ThrowsAnyAsync( + () => connector.OpenUpdateAsync(path, [this._directory], cancellation.Token)); + Assert.False(File.Exists(path)); + } + + [SymbolicLinkFact] + public async Task ConnectorRejectsOutsideLinksForReadsAndUpdatesAsync() + { + var allowed = Path.Combine(this._directory, "allowed"); + Directory.CreateDirectory(allowed); + var outside = Path.Combine(this._directory, "outside.txt"); + File.WriteAllText(outside, "unchanged"); + var link = Path.Combine(allowed, "link.txt"); + File.CreateSymbolicLink(link, outside); + var connector = new LocalFileSystemConnector(); + + await Assert.ThrowsAsync( + () => connector.GetFileContentStreamAsync(link, [allowed])); + await Assert.ThrowsAsync(() => connector.OpenUpdateAsync(link, [allowed])); + Assert.Equal("unchanged", File.ReadAllText(outside)); + } + + public void Dispose() + { + Directory.Delete(this._directory, recursive: true); + } +} diff --git a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs index c24e9bc3d39b..87c45c6a1692 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs @@ -12,17 +12,25 @@ namespace SemanticKernel.Plugins.UnitTests.MsGraph; -public class CloudDrivePluginTests +public sealed class CloudDrivePluginTests : IDisposable { + private readonly string _tempDirectory = Path.Combine(Path.GetTempPath(), $"CloudDrivePluginTests_{Guid.NewGuid():N}"); + + public CloudDrivePluginTests() + { + Directory.CreateDirectory(this._tempDirectory); + } + [Fact] public async Task UploadSmallFileAsyncSucceedsAsync() { // Arrange - string allowedDir = Path.GetTempPath(); + string allowedDir = this._tempDirectory; string anyFilePath = Path.Combine(allowedDir, Guid.NewGuid().ToString()); + await File.WriteAllTextAsync(anyFilePath, "upload content"); Mock connectorMock = new(); - connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) + connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask); CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir], AllowedUploadDestinationPaths = ["/"] }; @@ -201,11 +209,13 @@ await Assert.ThrowsAsync(async () => public async Task ItAllowsSubdirectoriesOfAllowedDirectoriesAsync() { // Arrange - var allowedDir = Path.GetTempPath(); + var allowedDir = this._tempDirectory; var subDirPath = Path.Combine(allowedDir, "subdir", "nested", "file.txt"); + Directory.CreateDirectory(Path.GetDirectoryName(subDirPath)!); + await File.WriteAllTextAsync(subDirPath, "nested content"); Mock connectorMock = new(); - connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) + connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask); CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir], AllowedUploadDestinationPaths = ["/"] }; @@ -221,16 +231,17 @@ public async Task ItAllowsSubdirectoriesOfAllowedDirectoriesAsync() public async Task ItExpandsEnvironmentVariablesAndValidatesAsync() { // Arrange — set a dedicated test env var to avoid platform-specific assumptions - var tempDir = Path.GetTempPath().TrimEnd(Path.DirectorySeparatorChar); + var tempDir = this._tempDirectory; var envVarName = "SK_TEST_UPLOAD_DIR"; var originalValue = Environment.GetEnvironmentVariable(envVarName); try { Environment.SetEnvironmentVariable(envVarName, tempDir); var envVarPath = Path.Combine($"%{envVarName}%", "testfile.txt"); + await File.WriteAllTextAsync(Path.Combine(tempDir, "testfile.txt"), "expanded content"); Mock connectorMock = new(); - connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) + connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask); CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [tempDir], AllowedUploadDestinationPaths = ["/"] }; @@ -277,11 +288,13 @@ await Assert.ThrowsAsync(async () => public async Task ItRespectsPlatformCaseSensitivityAsync() { // Arrange — use differently-cased allowed dir vs file path - var allowedDir = Path.Combine(Path.GetTempPath(), "AllowedFolder"); - var filePath = Path.Combine(Path.GetTempPath(), "allowedfolder", "file.txt"); + var allowedDir = Path.Combine(this._tempDirectory, "AllowedFolder"); + var filePath = Path.Combine(this._tempDirectory, "allowedfolder", "file.txt"); + Directory.CreateDirectory(allowedDir); + await File.WriteAllTextAsync(Path.Combine(allowedDir, "file.txt"), "case content"); Mock connectorMock = new(); - connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) + connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask); CloudDrivePlugin target = new(connectorMock.Object) { AllowedUploadDirectories = [allowedDir], AllowedUploadDestinationPaths = ["/"] }; @@ -566,11 +579,12 @@ await Assert.ThrowsAsync(async () => public async Task UploadFileAsyncAllowsDestinationInAllowedPathAsync() { // Arrange - string allowedDir = Path.GetTempPath(); + string allowedDir = this._tempDirectory; string anyFilePath = Path.Combine(allowedDir, Guid.NewGuid().ToString()); + await File.WriteAllTextAsync(anyFilePath, "destination content"); Mock connectorMock = new(); - connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) + connectorMock.Setup(c => c.UploadSmallFileAsync(It.IsAny(), It.IsAny(), It.IsAny())) .Returns(Task.CompletedTask); CloudDrivePlugin target = new(connectorMock.Object) @@ -621,4 +635,30 @@ await Assert.ThrowsAsync(async () => await target.UploadFileAsync(Path.Combine(Path.GetTempPath(), "file.txt"), "/Confidential/secret.docx")); connectorMock.VerifyNoOtherCalls(); } + + [SymbolicLinkFact] + public async Task UploadDoesNotFollowLinksOutsideAllowedDirectoryAsync() + { + var allowed = Path.Combine(this._tempDirectory, "allowed"); + Directory.CreateDirectory(allowed); + var outside = Path.Combine(this._tempDirectory, "outside.txt"); + await File.WriteAllTextAsync(outside, "outside"); + var link = Path.Combine(allowed, "link.txt"); + File.CreateSymbolicLink(link, outside); + var connector = new Mock(MockBehavior.Strict); + var plugin = new CloudDrivePlugin(connector.Object) + { + AllowedUploadDirectories = [allowed], + AllowedUploadDestinationPaths = ["/"] + }; + + await Assert.ThrowsAsync(() => plugin.UploadFileAsync(link, "/file.txt")); + connector.VerifyNoOtherCalls(); + } + + public void Dispose() + { + Directory.Delete(this._tempDirectory, recursive: true); + GC.SuppressFinalize(this); + } } diff --git a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/OneDriveConnectorTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/OneDriveConnectorTests.cs new file mode 100644 index 000000000000..8689b1cb392c --- /dev/null +++ b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/OneDriveConnectorTests.cs @@ -0,0 +1,69 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System.IO; +using System.Net; +using System.Net.Http; +using System.Text; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.Graph; +using Microsoft.Kiota.Abstractions.Authentication; +using Microsoft.SemanticKernel.Plugins.MsGraph.Connectors; +using Xunit; + +namespace SemanticKernel.Plugins.UnitTests.MsGraph; + +public sealed class OneDriveConnectorTests +{ + [Fact] + public async Task UploadUsesProvidedStreamAndLeavesItOpenAsync() + { + using var handler = new UploadHandler(); + using var client = new HttpClient(handler); + using var graph = new GraphServiceClient(client, new AnonymousAuthenticationProvider()); + var connector = new OneDriveConnector(graph); + using var content = new MemoryStream(Encoding.UTF8.GetBytes("prefix-content")); + content.Position = 7; + + await connector.UploadSmallFileAsync(content, "/file.txt"); + + Assert.Equal("content", Encoding.UTF8.GetString(handler.UploadedContent!)); + Assert.True(content.CanRead); + } + + [Fact] + public async Task OversizeStreamIsRejectedBeforeSendingRequestAsync() + { + using var handler = new UploadHandler(); + using var client = new HttpClient(handler); + using var graph = new GraphServiceClient(client, new AnonymousAuthenticationProvider()); + var connector = new OneDriveConnector(graph); + using var content = new MemoryStream(new byte[(4 * 1024 * 1024) + 1]); + + await Assert.ThrowsAsync(() => connector.UploadSmallFileAsync(content, "/file.txt")); + Assert.Equal(0, handler.RequestCount); + Assert.True(content.CanRead); + } + + private sealed class UploadHandler : HttpMessageHandler + { + internal byte[]? UploadedContent { get; private set; } + + internal int RequestCount { get; private set; } + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + this.RequestCount++; + if (request.Method == HttpMethod.Put) + { + this.UploadedContent = await request.Content!.ReadAsByteArrayAsync(cancellationToken); + } + + return new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("{\"id\":\"test-drive\"}", Encoding.UTF8, "application/json") + }; + } + } +} diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Plugins.UnitTests.csproj b/dotnet/src/Plugins/Plugins.UnitTests/Plugins.UnitTests.csproj index cd444b483e18..6344fbbc2b9d 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Plugins.UnitTests.csproj +++ b/dotnet/src/Plugins/Plugins.UnitTests/Plugins.UnitTests.csproj @@ -38,11 +38,6 @@ - - - - - Always diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs b/dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs new file mode 100644 index 000000000000..218fa80f7b56 --- /dev/null +++ b/dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs @@ -0,0 +1,64 @@ +// Copyright (c) Microsoft. All rights reserved. + +using System; +using System.IO; +using System.Runtime.InteropServices; +using Xunit; + +namespace SemanticKernel.Plugins.UnitTests; + +[AttributeUsage(AttributeTargets.Method)] +internal sealed class SymbolicLinkFactAttribute : FactAttribute +{ + public SymbolicLinkFactAttribute() + { + this.Skip = SymbolicLinkTestSupport.SkipReason; + } +} + +[AttributeUsage(AttributeTargets.Method)] +internal sealed class SymbolicLinkTheoryAttribute : TheoryAttribute +{ + public SymbolicLinkTheoryAttribute() + { + this.Skip = SymbolicLinkTestSupport.SkipReason; + } +} + +[AttributeUsage(AttributeTargets.Method)] +internal sealed class UnixFactAttribute : FactAttribute +{ + public UnixFactAttribute() + { + if (!RuntimeInformation.IsOSPlatform(OSPlatform.Linux) && + !RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) + { + this.Skip = "This test covers Unix special files."; + } + } +} + +internal static class SymbolicLinkTestSupport +{ + internal static string? SkipReason { get; } = GetSkipReason(); + + private static string? GetSkipReason() + { + var directory = Path.Combine(Path.GetTempPath(), $"SymbolicLinkProbe_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + try + { + File.CreateSymbolicLink(Path.Combine(directory, "file-link"), Path.Combine(directory, "target")); + Directory.CreateSymbolicLink(Path.Combine(directory, "directory-link"), directory); + return null; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + return $"Symbolic link creation is not permitted: {ex.Message}"; + } + finally + { + Directory.Delete(directory, recursive: true); + } + } +} diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs index a90989df4872..33911cb9c1a9 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs @@ -2,8 +2,10 @@ using System; using System.IO; +using System.Net; using System.Net.Http; using System.Runtime.InteropServices; +using System.Threading; using System.Threading.Tasks; using Microsoft.SemanticKernel; using Microsoft.SemanticKernel.Plugins.Web; @@ -257,6 +259,127 @@ public async Task DownloadToFileUsesCaseSensitiveAllowListComparisonOnLinuxAsync } } + [SymbolicLinkTheory] + [InlineData(false)] + [InlineData(true)] + public async Task DownloadRejectsLinksReplacedDuringRequestAsync(bool replaceParent) + { + var directory = Path.Combine(Path.GetTempPath(), $"WebFileDownloadPluginTests_{Guid.NewGuid():N}"); + var allowed = Path.Combine(directory, "allowed"); + var parent = Path.Combine(allowed, "parent"); + var outsideDirectory = Path.Combine(directory, "outside"); + Directory.CreateDirectory(parent); + Directory.CreateDirectory(outsideDirectory); + var path = Path.Combine(parent, "file.txt"); + var outside = Path.Combine(outsideDirectory, "file.txt"); + await File.WriteAllTextAsync(path, "allowed"); + await File.WriteAllTextAsync(outside, "outside"); + try + { + using var handler = new CallbackHandler(() => + { + if (replaceParent) + { + Directory.Move(parent, parent + "-original"); + Directory.CreateSymbolicLink(parent, outsideDirectory); + } + else + { + File.Delete(path); + File.CreateSymbolicLink(path, outside); + } + + return new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent("replacement") }; + }); + using var client = new HttpClient(handler); + var plugin = new WebFileDownloadPlugin(client) + { + AllowedDomains = ["localhost"], + AllowedFolders = [allowed], + DisableFileOverwrite = false + }; + + await Assert.ThrowsAsync( + () => plugin.DownloadToFileAsync(new Uri("http://localhost/file"), path)); + Assert.Equal("outside", await File.ReadAllTextAsync(outside)); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + + [Fact] + public async Task DownloadDoesNotOverwriteFileCreatedDuringRequestAsync() + { + var directory = Path.Combine(Path.GetTempPath(), $"WebFileDownloadPluginTests_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + var path = Path.Combine(directory, "file.txt"); + try + { + using var handler = new CallbackHandler(() => + { + File.WriteAllText(path, "created during request"); + return new HttpResponseMessage(HttpStatusCode.OK) { Content = new StringContent("replacement") }; + }); + using var client = new HttpClient(handler); + var plugin = new WebFileDownloadPlugin(client) + { + AllowedDomains = ["localhost"], + AllowedFolders = [directory] + }; + + var error = await Assert.ThrowsAsync( + () => plugin.DownloadToFileAsync(new Uri("http://localhost/file"), path)); + Assert.Contains("Overwriting", error.Message); + Assert.Equal("created during request", await File.ReadAllTextAsync(path)); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + + [Theory] + [InlineData(HttpStatusCode.Forbidden, 10)] + [InlineData(HttpStatusCode.OK, 1)] + public async Task RejectedResponseDoesNotTruncateDestinationAsync(HttpStatusCode status, int maximumSize) + { + var directory = Path.Combine(Path.GetTempPath(), $"WebFileDownloadPluginTests_{Guid.NewGuid():N}"); + Directory.CreateDirectory(directory); + var path = Path.Combine(directory, "file.txt"); + await File.WriteAllTextAsync(path, "unchanged"); + try + { + using var handler = new CallbackHandler(() => new HttpResponseMessage(status) + { + Content = new StringContent("oversize") + }); + using var client = new HttpClient(handler); + var plugin = new WebFileDownloadPlugin(client) + { + AllowedDomains = ["localhost"], + AllowedFolders = [directory], + DisableFileOverwrite = false, + MaximumDownloadSize = maximumSize + }; + + await Assert.ThrowsAnyAsync( + () => plugin.DownloadToFileAsync(new Uri("http://localhost/file"), path)); + Assert.Equal("unchanged", await File.ReadAllTextAsync(path)); + } + finally + { + Directory.Delete(directory, recursive: true); + } + } + + private sealed class CallbackHandler(Func responseFactory) : HttpMessageHandler + { + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) => + Task.FromResult(responseFactory()); + } + /// public void Dispose() { diff --git a/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs b/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs index f08482c0d124..520ad756433b 100644 --- a/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Web/WebFileDownloadPlugin.cs @@ -155,7 +155,7 @@ public async Task DownloadToFileAsync( var fileMode = this.DisableFileOverwrite ? FileMode.CreateNew : FileMode.Create; using Stream source = await response.Content.ReadAsStreamAndTranslateExceptionAsync(cancellationToken).ConfigureAwait(false); - using FileStream destination = new(expandedFilePath, fileMode); + using FileStream destination = this.OpenDestination(expandedFilePath, fileMode); int bufferSize = 81920; byte[] buffer = ArrayPool.Shared.Rent(81920); @@ -193,6 +193,18 @@ public async Task DownloadToFileAsync( private HashSet? _allowedDomains = []; private HashSet? _allowedFolders = []; + private FileStream OpenDestination(string path, FileMode mode) + { + try + { + return SecureFileSystem.OpenWrite(path, this._allowedFolders, out _, mode, FileShare.None); + } + catch (SecureFileSystem.FileAlreadyExistsException ex) + { + throw new InvalidOperationException("Overwriting existing files is disabled.", ex); + } + } + /// /// If a list of allowed domains has been provided, the host of the provided uri is checked /// to verify it is in the allowed domain list. @@ -208,29 +220,7 @@ private bool IsUriAllowed(Uri uri) private static string CanonicalizePath(string path) { - Verify.NotNullOrWhiteSpace(path); - - if (IsUncOrExtendedPath(path)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); - } - - var expanded = Environment.ExpandEnvironmentVariables(path); - if (IsUncOrExtendedPath(expanded)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); - } - - // Resolve the full path first (a pure string operation that does not touch the - // filesystem). A relative path can still resolve to a UNC path here, for example - // when the current directory is a UNC share, so re-check before GetSafeFullPath - // probes the filesystem while resolving symbolic links. - var fullPath = Path.GetFullPath(expanded); - if (IsUncOrExtendedPath(fullPath)) - { - throw new ArgumentException("Invalid file path, UNC paths are not supported.", nameof(path)); - } - + var fullPath = SecureFileSystem.NormalizePath(path, expandEnvironmentVariables: true); return PathUtilities.GetSafeFullPath(fullPath); } @@ -260,12 +250,6 @@ private bool IsFilePathAllowed(string path) throw new ArgumentException("Invalid file path, a fully qualified file location must be specified.", nameof(path)); } - if (File.Exists(path) && File.GetAttributes(path).HasFlag(FileAttributes.ReadOnly)) - { - // Most environments will throw this with OpenWrite, but running inside docker on Linux will not. - throw new UnauthorizedAccessException($"File is read-only: {path}"); - } - if (this._allowedFolders is null || this._allowedFolders.Count == 0) { return false; From 50b10df255a6a64459f68e8a4b254e7df4e80f21 Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Thu, 29 Oct 2026 16:43:32 +0000 Subject: [PATCH 2/7] Fix build --- .../workflows/dotnet-secure-file-tests.yml | 2 +- .../src/System/SecureFileSystem.cs | 30 +++++++++++++++---- .../Core/PathUtilitiesTests.cs | 2 +- .../Document/DocumentPluginTests.cs | 10 +++++-- .../Web/WebFileDownloadPluginTests.cs | 4 +-- 5 files changed, 36 insertions(+), 12 deletions(-) diff --git a/.github/workflows/dotnet-secure-file-tests.yml b/.github/workflows/dotnet-secure-file-tests.yml index 32383fdf94bb..d9ad624f571c 100644 --- a/.github/workflows/dotnet-secure-file-tests.yml +++ b/.github/workflows/dotnet-secure-file-tests.yml @@ -30,4 +30,4 @@ jobs: - name: Test secure file operations run: >- dotnet test dotnet/src/Plugins/Plugins.UnitTests/Plugins.UnitTests.csproj - --filter "FullyQualifiedName~SecureFileSystemTests|FullyQualifiedName~FileIOPluginTests|FullyQualifiedName~SessionsPythonPluginTests|FullyQualifiedName~WebFileDownloadPluginTests|FullyQualifiedName~CloudDrivePluginTests|FullyQualifiedName~DocumentPluginTests|FullyQualifiedName~LocalFileSystemConnectorTests|FullyQualifiedName~OneDriveConnectorTests" + --filter "FullyQualifiedName~SecureFileSystemTests|FullyQualifiedName~FileIOPluginTests|FullyQualifiedName~SessionsPythonPluginTests|FullyQualifiedName~WebFileDownloadPluginTests|FullyQualifiedName~CloudDrivePluginTests|FullyQualifiedName~DocumentPluginTests|FullyQualifiedName~LocalFileSystemConnectorTests|FullyQualifiedName~OneDriveConnectorTests|FullyQualifiedName~PathUtilitiesTests" diff --git a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs index fbf9044b96f2..6dc68f7fb88c 100644 --- a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs +++ b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs @@ -232,7 +232,7 @@ internal static SafeFileHandle Open( { created = false; var directoryFlags = GetDirectoryFlags(); - var currentHandle = OpenPath("/", directoryFlags, 0); + var currentHandle = OpenPath("/", directoryFlags); try { @@ -347,9 +347,9 @@ private static string[] GetSegmentsAfterRoot(string path) StringSplitOptions.RemoveEmptyEntries); } - private static SafeFileHandle OpenPath(string path, int flags, uint mode) + private static SafeFileHandle OpenPath(string path, int flags) { - var descriptor = OpenNative(path, flags, mode); + var descriptor = OpenNative(path, flags); return CreateHandle(descriptor, path); } @@ -359,6 +359,21 @@ private static SafeFileHandle OpenAt(SafeFileHandle directory, string path, int return CreateHandle(descriptor, path); } + private static int OpenAtNative(SafeFileHandle directory, string path, int flags, uint mode) + { + if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX) && + RuntimeInformation.ProcessArchitecture == Architecture.Arm64) + { + // Apple's ARM64 openat reads its optional permissions argument from the stack, + // not a register. Fill the five remaining argument registers so this fixed + // P/Invoke signature puts mode on the stack, where the native function expects it. + return OpenAtMacArm64(directory, path, flags, + IntPtr.Zero, IntPtr.Zero, IntPtr.Zero, IntPtr.Zero, IntPtr.Zero, mode); + } + + return OpenAtDefault(directory, path, flags, mode); + } + private static void CheckFile(SafeFileHandle handle, bool rejectReadOnly) { ushort mode; @@ -429,10 +444,15 @@ private static SafeFileHandle CreateHandle(int descriptor, string path) } [DllImport("libc", EntryPoint = "open", SetLastError = true)] - private static extern int OpenNative(string path, int flags, uint mode); + private static extern int OpenNative(string path, int flags); + + [DllImport("libc", EntryPoint = "openat", SetLastError = true)] + private static extern int OpenAtDefault(SafeFileHandle directoryDescriptor, string path, int flags, uint mode); [DllImport("libc", EntryPoint = "openat", SetLastError = true)] - private static extern int OpenAtNative(SafeFileHandle directoryDescriptor, string path, int flags, uint mode); + private static extern int OpenAtMacArm64( + SafeFileHandle directoryDescriptor, string path, int flags, + IntPtr unused3, IntPtr unused4, IntPtr unused5, IntPtr unused6, IntPtr unused7, uint mode); [DllImport("libc", EntryPoint = "flock", SetLastError = true)] private static extern int Flock(SafeFileHandle descriptor, int operation); diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/PathUtilitiesTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/PathUtilitiesTests.cs index 8b6626e7c6b6..6d1547af482b 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/PathUtilitiesTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/PathUtilitiesTests.cs @@ -13,7 +13,7 @@ public sealed class PathUtilitiesTests : IDisposable public PathUtilitiesTests() { - this._tempDir = Path.Combine(Path.GetTempPath(), $"PathUtilitiesTests_{Guid.NewGuid():N}"); + this._tempDir = Path.Combine(PathUtilities.GetSafeFullPath(Path.GetTempPath()), $"PathUtilitiesTests_{Guid.NewGuid():N}"); Directory.CreateDirectory(this._tempDir); } diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs index 17f949af60cf..25865e8e0854 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs @@ -5,6 +5,7 @@ using System.IO; using System.Threading; using System.Threading.Tasks; +using Microsoft.SemanticKernel; using Microsoft.SemanticKernel.Plugins.Document; using Microsoft.SemanticKernel.Plugins.Document.FileSystem; using Moq; @@ -21,10 +22,11 @@ public async Task ReadTextAsyncSucceedsAsync() var expectedText = Guid.NewGuid().ToString(); var folderPath = Path.GetTempPath(); var anyFilePath = Path.Combine(folderPath, Guid.NewGuid().ToString()); + var expectedFilePath = PathUtilities.GetSafeFullPath(anyFilePath); var fileSystemConnectorMock = new Mock(); fileSystemConnectorMock - .Setup(mock => mock.GetFileContentStreamAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), + .Setup(mock => mock.GetFileContentStreamAsync(It.Is(filePath => filePath.Equals(expectedFilePath, StringComparison.Ordinal)), It.IsAny>(), It.IsAny())) .ReturnsAsync(Stream.Null); @@ -55,10 +57,11 @@ public async Task AppendTextAsyncFileExistsSucceedsAsync() var anyText = Guid.NewGuid().ToString(); var folderPath = Path.GetTempPath(); var anyFilePath = Path.Combine(folderPath, Guid.NewGuid().ToString()); + var expectedFilePath = PathUtilities.GetSafeFullPath(anyFilePath); var fileSystemConnectorMock = new Mock(); fileSystemConnectorMock - .Setup(mock => mock.OpenUpdateAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), + .Setup(mock => mock.OpenUpdateAsync(It.Is(filePath => filePath.Equals(expectedFilePath, StringComparison.Ordinal)), It.IsAny>(), It.IsAny())) .ReturnsAsync((Stream.Null, false)); @@ -87,10 +90,11 @@ public async Task AppendTextAsyncFileDoesNotExistSucceedsAsync() var anyText = Guid.NewGuid().ToString(); var folderPath = Path.GetTempPath(); var anyFilePath = Path.Combine(folderPath, Guid.NewGuid().ToString()); + var expectedFilePath = PathUtilities.GetSafeFullPath(anyFilePath); var fileSystemConnectorMock = new Mock(); fileSystemConnectorMock - .Setup(mock => mock.OpenUpdateAsync(It.Is(filePath => filePath.Equals(anyFilePath, StringComparison.Ordinal)), + .Setup(mock => mock.OpenUpdateAsync(It.Is(filePath => filePath.Equals(expectedFilePath, StringComparison.Ordinal)), It.IsAny>(), It.IsAny())) .ReturnsAsync((Stream.Null, true)); diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs index 33911cb9c1a9..b76e0ccd802a 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Web/WebFileDownloadPluginTests.cs @@ -37,7 +37,7 @@ public async Task DownloadToFileSucceedsAsync() var filePath = Path.Combine(folderPath, "sk_logo.png"); Directory.CreateDirectory(folderPath); - var webFileDownload = new WebFileDownloadPlugin() + var webFileDownload = new WebFileDownloadPlugin(this._httpClient) { AllowedDomains = ["raw.githubusercontent.com"], AllowedFolders = [folderPath] @@ -180,7 +180,7 @@ public async Task DownloadToFileFailsForInvalidParametersAsync() var invalidFilePath = Path.Combine(Path.GetTempPath(), "sk_logo.png"); Directory.CreateDirectory(folderPath); - var webFileDownload = new WebFileDownloadPlugin() + var webFileDownload = new WebFileDownloadPlugin(this._httpClient) { AllowedDomains = ["raw.githubusercontent.com"], AllowedFolders = [folderPath], From d1e1ca6d3ef566d6220bf10ddbd4de40a6961bde Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Thu, 29 Oct 2026 17:14:02 +0000 Subject: [PATCH 3/7] Address PR comments --- .../src/System/SecureFileSystem.cs | 5 +- .../Plugins.MsGraph/CloudDrivePlugin.cs | 28 ++++-- .../Core/SecureFileSystemTests.cs | 48 ++++++++++ .../MsGraph/CloudDrivePluginTests.cs | 90 +++++++++++++++++++ .../Support/SymbolicLinkFactAttribute.cs | 31 +++++++ 5 files changed, 194 insertions(+), 8 deletions(-) diff --git a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs index 6dc68f7fb88c..0c2865bd8a9e 100644 --- a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs +++ b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs @@ -217,6 +217,7 @@ private static class UnixFileSystem private const int ReadOnly = 0; private const int WriteOnly = 1; private const int ReadWrite = 2; + private const int LinuxPathOnly = 0x200000; private const uint DefaultPermissions = 0x1B6; // 0666, reduced by the process umask. private const int AlreadyExists = 17; @@ -319,7 +320,9 @@ internal static SafeFileHandle Open( #pragma warning restore CA2000 private static int GetDirectoryFlags() => - ReadOnly | GetDirectoryFlag() | GetNoFollowFlag() | GetCloseOnExecFlag(); + // Traversal needs search permission, not permission to list each ancestor. + (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) ? LinuxPathOnly : ReadOnly) | + GetDirectoryFlag() | GetNoFollowFlag() | GetCloseOnExecFlag(); private static int GetDirectoryFlag() => RuntimeInformation.IsOSPlatform(OSPlatform.OSX) ? 0x100000 : 0x10000; diff --git a/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs b/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs index 53e78378a1e7..dbfe2f7fb1c6 100644 --- a/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs +++ b/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs @@ -5,6 +5,7 @@ using System.ComponentModel; using System.IO; using System.Runtime.InteropServices; +using System.Security; using System.Threading; using System.Threading.Tasks; using Microsoft.Extensions.Logging; @@ -176,9 +177,7 @@ public async Task UploadFileAsync( Ensure.NotNullOrWhitespace(filePath, nameof(filePath)); - var canonicalPath = CanonicalizePath(filePath); - - if (!this.IsUploadPathAllowed(canonicalPath)) + if (!this.TryGetAllowedUploadPath(filePath, out var canonicalPath)) { throw new InvalidOperationException("Uploading from the provided location is not allowed. Configure 'AllowedUploadDirectories' with trusted directory paths to enable uploads."); } @@ -293,13 +292,28 @@ private static string NormalizeRemotePath(string path) } /// - /// Expands environment variables and resolves the path to its canonical form. + /// Validates the upload location without exposing caller-path resolution errors. /// The upload still uses a secure open because this path can change after validation. /// - private static string CanonicalizePath(string path) + private bool TryGetAllowedUploadPath(string path, out string canonicalPath) { - var fullPath = SecureFileSystem.NormalizePath(path, expandEnvironmentVariables: true); - return PathUtilities.GetSafeFullPath(fullPath); + canonicalPath = SecureFileSystem.NormalizePath(path, expandEnvironmentVariables: true); + if (this._allowedUploadDirectories.Count == 0) + { + return false; + } + + try + { + canonicalPath = PathUtilities.GetSafeFullPath(canonicalPath); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or InvalidOperationException or SecurityException) + { + return false; + } + + // Keep trusted configuration errors outside the caller-path error handling. + return this.IsUploadPathAllowed(canonicalPath); } /// diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs index 7b8dd353ea4e..26f7da96ff6e 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/SecureFileSystemTests.cs @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.IO; using System.Runtime.InteropServices; +using System.Runtime.Versioning; using System.Text; using System.Threading.Tasks; using Microsoft.SemanticKernel; @@ -389,6 +390,53 @@ public void MissingParentIsNotCreated() Assert.False(Directory.Exists(Path.GetDirectoryName(path))); } + [LinuxNonRootTheory] + [InlineData(false)] + [InlineData(true)] + [SupportedOSPlatform("linux")] + public void SearchOnlyDirectoriesDoNotPreventFileAccess(bool restrictedAncestorOfAllowedRoot) + { + var restricted = Path.Combine(this._tempDirectory, "search-only"); + var writable = Path.Combine(restricted, "writable"); + Directory.CreateDirectory(writable); + var allowed = restrictedAncestorOfAllowedRoot ? writable : restricted; + var input = Path.Combine(writable, "input.txt"); + var output = Path.Combine(writable, "output.txt"); + File.WriteAllText(input, "readable"); + var originalMode = File.GetUnixFileMode(restricted); + try + { + File.SetUnixFileMode(restricted, + UnixFileMode.UserExecute | UnixFileMode.GroupExecute | UnixFileMode.OtherExecute); + Assert.Equal("readable", File.ReadAllText(input)); + Assert.Throws(() => Directory.GetFileSystemEntries(restricted)); + + using (var stream = SecureFileSystem.OpenRead(input, [allowed], out _)) + using (var reader = new StreamReader(stream)) + { + Assert.Equal("readable", reader.ReadToEnd()); + } + + using (var stream = SecureFileSystem.OpenWrite(output, [allowed], out _, FileMode.CreateNew)) + { + stream.WriteByte(42); + } + + using (var stream = SecureFileSystem.OpenUpdate(output, [allowed], out var created)) + { + Assert.False(created); + Assert.Equal(42, stream.ReadByte()); + stream.WriteByte(43); + } + + Assert.Equal(new byte[] { 42, 43 }, File.ReadAllBytes(output)); + } + finally + { + File.SetUnixFileMode(restricted, originalMode); + } + } + [UnixFact(Timeout = 5000)] public async Task NamedPipeIsRejectedWithoutWaitingForAnotherProcessAsync() { diff --git a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs index 87c45c6a1692..14408b3b71ea 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs @@ -3,9 +3,11 @@ using System; using System.IO; using System.Runtime.InteropServices; +using System.Runtime.Versioning; using System.Text; using System.Threading; using System.Threading.Tasks; +using Microsoft.SemanticKernel; using Microsoft.SemanticKernel.Plugins.MsGraph; using Moq; using Xunit; @@ -656,6 +658,94 @@ public async Task UploadDoesNotFollowLinksOutsideAllowedDirectoryAsync() connector.VerifyNoOtherCalls(); } + [SymbolicLinkTheory] + [InlineData(false)] + [InlineData(true)] + public async Task UploadPathResolutionFailuresReturnTheUsualDenialAsync(bool emptyAllowedDirectories) + { + var allowed = Path.Combine(this._tempDirectory, "allowed"); + Directory.CreateDirectory(allowed); + var loop = Path.Combine(this._tempDirectory, "loop"); + Directory.CreateSymbolicLink(loop, loop); + Assert.Throws(() => PathUtilities.GetSafeFullPath(loop)); + var connector = new Mock(MockBehavior.Strict); + var plugin = new CloudDrivePlugin(connector.Object) + { + AllowedUploadDirectories = emptyAllowedDirectories ? [] : [allowed], + AllowedUploadDestinationPaths = ["/"] + }; + var expected = await Assert.ThrowsAsync( + () => plugin.UploadFileAsync(Path.Combine(this._tempDirectory, "missing.txt"), "/file.txt")); + + var actual = await Assert.ThrowsAsync( + () => plugin.UploadFileAsync(Path.Combine(loop, "file.txt"), "/file.txt")); + + Assert.Equal(expected.Message, actual.Message); + Assert.Null(actual.InnerException); + connector.VerifyNoOtherCalls(); + } + + [SymbolicLinkFact] + public async Task UploadPathResolutionFailuresDoNotHideConfigurationErrorsAsync() + { + var loop = Path.Combine(this._tempDirectory, "loop"); + Directory.CreateSymbolicLink(loop, loop); + var expected = Assert.Throws(() => PathUtilities.GetSafeFullPath(loop)); + var connector = new Mock(MockBehavior.Strict); + var plugin = new CloudDrivePlugin(connector.Object) + { + AllowedUploadDirectories = [loop], + AllowedUploadDestinationPaths = ["/"] + }; + + var actual = await Assert.ThrowsAsync( + () => plugin.UploadFileAsync(Path.Combine(this._tempDirectory, "file.txt"), "/file.txt")); + + Assert.Equal(expected.Message, actual.Message); + connector.VerifyNoOtherCalls(); + } + + [LinuxNonRootFact] + [SupportedOSPlatform("linux")] + public async Task InaccessibleUploadPathsDoNotRevealDetailsOrHideConfigurationErrorsAsync() + { + var allowed = Path.Combine(this._tempDirectory, "allowed"); + var restricted = Path.Combine(this._tempDirectory, "restricted"); + var nested = Path.Combine(restricted, "nested"); + Directory.CreateDirectory(allowed); + Directory.CreateDirectory(nested); + var inaccessible = Path.Combine(nested, "file.txt"); + await File.WriteAllTextAsync(inaccessible, "outside"); + var connector = new Mock(MockBehavior.Strict); + var plugin = new CloudDrivePlugin(connector.Object) + { + AllowedUploadDirectories = [allowed], + AllowedUploadDestinationPaths = ["/"] + }; + var expected = await Assert.ThrowsAsync( + () => plugin.UploadFileAsync(Path.Combine(this._tempDirectory, "missing.txt"), "/file.txt")); + var originalMode = File.GetUnixFileMode(restricted); + try + { + File.SetUnixFileMode(restricted, UnixFileMode.None); + Assert.Throws(() => PathUtilities.GetSafeFullPath(inaccessible)); + + var actual = await Assert.ThrowsAsync( + () => plugin.UploadFileAsync(inaccessible, "/file.txt")); + Assert.Equal(expected.Message, actual.Message); + Assert.Null(actual.InnerException); + + plugin.AllowedUploadDirectories = [nested]; + await Assert.ThrowsAsync( + () => plugin.UploadFileAsync(Path.Combine(allowed, "file.txt"), "/file.txt")); + connector.VerifyNoOtherCalls(); + } + finally + { + File.SetUnixFileMode(restricted, originalMode); + } + } + public void Dispose() { Directory.Delete(this._tempDirectory, recursive: true); diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs b/dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs index 218fa80f7b56..8b28735eadf7 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Support/SymbolicLinkFactAttribute.cs @@ -38,6 +38,37 @@ public UnixFactAttribute() } } +[AttributeUsage(AttributeTargets.Method)] +internal sealed class LinuxNonRootFactAttribute : FactAttribute +{ + public LinuxNonRootFactAttribute() + { + this.Skip = LinuxNonRootTestSupport.SkipReason; + } +} + +[AttributeUsage(AttributeTargets.Method)] +internal sealed class LinuxNonRootTheoryAttribute : TheoryAttribute +{ + public LinuxNonRootTheoryAttribute() + { + this.Skip = LinuxNonRootTestSupport.SkipReason; + } +} + +internal static class LinuxNonRootTestSupport +{ + internal static string? SkipReason { get; } = + !RuntimeInformation.IsOSPlatform(OSPlatform.Linux) + ? "This test requires Linux directory permissions." + : GetEffectiveUserId() == 0 + ? "Root can bypass the directory permissions this test needs to enforce." + : null; + + [DllImport("libc", EntryPoint = "geteuid")] + private static extern uint GetEffectiveUserId(); +} + internal static class SymbolicLinkTestSupport { internal static string? SkipReason { get; } = GetSkipReason(); From d6299e4a3bd2bb44917ded7a0b70b4ee5edb32c5 Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Thu, 29 Oct 2026 17:31:04 +0000 Subject: [PATCH 4/7] Fix code coverage issue --- dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs index 0c2865bd8a9e..14f25d8c9182 100644 --- a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs +++ b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs @@ -2,6 +2,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.IO; using System.Runtime.InteropServices; using System.Security; @@ -19,6 +20,7 @@ namespace Microsoft.SemanticKernel; /// This prevents link replacement, not changes to mounts, hard links, or the contents of an open file. /// Linux file inspection requires libc/kernel support for statx; failure never falls back to a path check. /// +[ExcludeFromCodeCoverage] internal static class SecureFileSystem { internal static FileStream OpenRead( From 93a974d1aaae671124e1e00e95a55d88839f45ca Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:15:59 +0000 Subject: [PATCH 5/7] Add more improvements --- .../Plugins.MsGraph/CloudDrivePlugin.cs | 83 +++++++-- .../MsGraph/CloudDrivePluginTests.cs | 172 ++++++++++++++++++ .../MsGraph/OneDriveConnectorTests.cs | 136 ++++++++++++++ 3 files changed, 371 insertions(+), 20 deletions(-) diff --git a/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs b/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs index dbfe2f7fb1c6..c4d76e003484 100644 --- a/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs +++ b/dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs @@ -74,7 +74,8 @@ public IEnumerable AllowedUploadDirectories /// /// Defaults to an empty collection (no paths allowed). Must be explicitly populated /// with trusted remote directory paths before any share-link operations will succeed. - /// Paths are normalized with forward slashes and dot-segments are collapsed before comparison. + /// Paths are normalized with forward slashes and literal dot-segments are collapsed before comparison. + /// Encoded dot-segments are rejected because URI construction could otherwise reinterpret the checked path. /// Matching is case-insensitive (OneDrive paths are case-insensitive). /// public IEnumerable AllowedSharePaths @@ -91,7 +92,8 @@ public IEnumerable AllowedSharePaths /// /// Defaults to an empty collection (no paths allowed). Must be explicitly populated /// with trusted remote directory paths before any read operations will succeed. - /// Paths are normalized with forward slashes and dot-segments are collapsed before comparison. + /// Paths are normalized with forward slashes and literal dot-segments are collapsed before comparison. + /// Encoded dot-segments are rejected because URI construction could otherwise reinterpret the checked path. /// Matching is case-insensitive (OneDrive paths are case-insensitive). /// public IEnumerable AllowedReadPaths @@ -108,7 +110,8 @@ public IEnumerable AllowedReadPaths /// /// Defaults to an empty collection (no paths allowed). Must be explicitly populated /// with trusted remote directory paths before any upload-destination operations will succeed. - /// Paths are normalized with forward slashes and dot-segments are collapsed before comparison. + /// Paths are normalized with forward slashes and literal dot-segments are collapsed before comparison. + /// Encoded dot-segments are rejected because URI construction could otherwise reinterpret the checked path. /// Matching is case-insensitive (OneDrive paths are case-insensitive). /// public IEnumerable AllowedUploadDestinationPaths @@ -132,12 +135,12 @@ public IEnumerable AllowedUploadDestinationPaths Ensure.NotNullOrWhitespace(filePath, nameof(filePath)); - if (!this.IsAllowedRemotePath(filePath, this._allowedReadPaths)) + if (!this.TryGetAllowedRemotePath(filePath, this._allowedReadPaths, out var normalizedPath)) { throw new InvalidOperationException("Reading from the provided path is not allowed. Configure 'AllowedReadPaths' with trusted remote paths to enable reading."); } - Stream? fileContentStream = await this._connector.GetFileContentStreamAsync(filePath, cancellationToken).ConfigureAwait(false); + Stream? fileContentStream = await this._connector.GetFileContentStreamAsync(normalizedPath, cancellationToken).ConfigureAwait(false); if (fileContentStream is null) { @@ -170,7 +173,7 @@ public async Task UploadFileAsync( throw new ArgumentException("Variable was null or whitespace", nameof(destinationPath)); } - if (!this.IsAllowedRemotePath(destinationPath, this._allowedUploadDestinationPaths)) + if (!this.TryGetAllowedRemotePath(destinationPath, this._allowedUploadDestinationPaths, out var normalizedDestinationPath)) { throw new InvalidOperationException("Uploading to the provided remote destination is not allowed. Configure 'AllowedUploadDestinationPaths' with trusted remote paths to enable uploads."); } @@ -187,7 +190,7 @@ public async Task UploadFileAsync( using var content = SecureFileSystem.OpenRead(canonicalPath, this._allowedUploadDirectories, out _); // TODO Add support for large file uploads (i.e. upload sessions) - await this._connector.UploadSmallFileAsync(content, destinationPath, cancellationToken).ConfigureAwait(false); + await this._connector.UploadSmallFileAsync(content, normalizedDestinationPath, cancellationToken).ConfigureAwait(false); } /// @@ -207,12 +210,12 @@ public async Task CreateLinkAsync( Ensure.NotNullOrWhitespace(filePath, nameof(filePath)); - if (!this.IsAllowedRemotePath(filePath, this._allowedSharePaths)) + if (!this.TryGetAllowedRemotePath(filePath, this._allowedSharePaths, out var normalizedPath)) { throw new InvalidOperationException("Creating a share link for the provided path is not allowed. Configure 'AllowedSharePaths' with trusted remote paths to enable sharing."); } - return await this._connector.CreateShareLinkAsync(filePath, Type, Scope, cancellationToken).ConfigureAwait(false); + return await this._connector.CreateShareLinkAsync(normalizedPath, Type, Scope, cancellationToken).ConfigureAwait(false); } #region private @@ -224,25 +227,34 @@ public async Task CreateLinkAsync( /// /// Checks whether the provided remote path falls within one of the allowed remote directory prefixes. - /// Paths are normalized with forward slashes, dot-segments are collapsed, + /// Paths are normalized with forward slashes, literal dot-segments are collapsed, + /// encoded dot-segments are rejected, /// and compared case-insensitively (OneDrive paths are case-insensitive). /// Subdirectories of allowed paths are permitted. /// - private bool IsAllowedRemotePath(string path, HashSet allowedPaths) + private bool TryGetAllowedRemotePath(string path, HashSet allowedPaths, out string normalizedPath) { Ensure.NotNullOrWhitespace(path, nameof(path)); + normalizedPath = string.Empty; if (allowedPaths.Count == 0) { return false; } - // Normalize to forward slashes and collapse dot-segments to prevent traversal bypass. - var normalizedPath = NormalizeRemotePath(path); + if (!TryNormalizeRemotePath(path, out normalizedPath)) + { + return false; + } foreach (var allowedPath in allowedPaths) { - var normalizedAllowed = NormalizeRemotePath(allowedPath); + if (!TryNormalizeRemotePath(allowedPath, out var normalizedAllowed)) + { + throw new InvalidOperationException( + $"Configured allowed remote path '{allowedPath}' contains an encoded dot-segment."); + } + if (!normalizedAllowed.EndsWith("/", StringComparison.Ordinal)) { normalizedAllowed += "/"; @@ -267,14 +279,44 @@ private bool IsAllowedRemotePath(string path, HashSet allowedPaths) /// /// Normalizes a remote path by replacing backslashes with forward slashes - /// and collapsing "." and ".." segments to prevent traversal bypass. + /// and collapsing literal "." and ".." segments. /// - private static string NormalizeRemotePath(string path) + private static bool TryNormalizeRemotePath(string path, out string normalizedPath) { - var normalizedPath = path.Replace('\\', '/'); + // URI construction can decode percent-encoded dots and separators after the + // allow-list check. Reject any encoding that can reveal a structural dot-segment, + // then reuse the returned path for the connector call. + foreach (var rawSegment in path.Split('/')) + { + var decoded = rawSegment; + for (int i = 0; i < 5; i++) + { + var unescaped = Uri.UnescapeDataString(decoded); + if (string.Equals(unescaped, decoded, StringComparison.Ordinal)) + { + break; + } + + decoded = unescaped; + } + + if (!string.Equals(decoded, rawSegment, StringComparison.Ordinal)) + { + foreach (var segment in decoded.Split('/', '\\')) + { + if (segment is "." or "..") + { + normalizedPath = string.Empty; + return false; + } + } + } + } + + var pathWithNormalizedSeparators = path.Replace('\\', '/'); - // Collapse ".." and "." segments to prevent traversal bypass. - var segments = normalizedPath.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries); + // Collapse literal dot-segments so validation and connector use the same path. + var segments = pathWithNormalizedSeparators.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries); var stack = new List(); foreach (var segment in segments) { @@ -288,7 +330,8 @@ private static string NormalizeRemotePath(string path) } } - return "/" + string.Join("/", stack); + normalizedPath = "/" + string.Join("/", stack); + return true; } /// diff --git a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs index 14408b3b71ea..4de5692b991e 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs @@ -658,6 +658,112 @@ public async Task UploadDoesNotFollowLinksOutsideAllowedDirectoryAsync() connector.VerifyNoOtherCalls(); } + [Theory] + [MemberData(nameof(DangerousRemotePaths))] + public async Task RemoteOperationsRejectEncodedTraversalAsync(string operation, string remotePath) + { + var connector = new Mock(MockBehavior.Strict); + var plugin = new CloudDrivePlugin(connector.Object) + { + AllowedReadPaths = ["/Uploads"], + AllowedSharePaths = ["/Uploads"], + AllowedUploadDestinationPaths = ["/Uploads"] + }; + + await Assert.ThrowsAsync( + () => InvokeRemoteOperationAsync(plugin, operation, remotePath, Path.Combine(this._tempDirectory, "source.txt"))); + connector.VerifyNoOtherCalls(); + } + + [Theory] + [MemberData(nameof(AllowedNormalizedRemotePaths))] + public async Task RemoteOperationsUseTheCheckedNormalizedPathAsync( + string operation, + string suppliedPath, + string expectedPath) + { + var sourcePath = Path.Combine(this._tempDirectory, "source.txt"); + await File.WriteAllTextAsync(sourcePath, "content"); + var connector = new Mock(MockBehavior.Strict); + switch (operation) + { + case "read": + connector.Setup(c => c.GetFileContentStreamAsync(expectedPath, It.IsAny())) + .ReturnsAsync(new MemoryStream(Encoding.UTF8.GetBytes("content"))); + break; + case "share": + connector.Setup(c => c.CreateShareLinkAsync(expectedPath, "view", "organization", It.IsAny())) + .ReturnsAsync("https://example.com/share"); + break; + case "upload": + connector.Setup(c => c.UploadSmallFileAsync(It.IsAny(), expectedPath, It.IsAny())) + .Returns(Task.CompletedTask); + break; + default: + throw new InvalidOperationException($"Unknown test operation '{operation}'."); + } + + var plugin = new CloudDrivePlugin(connector.Object) + { + AllowedReadPaths = ["/Uploads"], + AllowedSharePaths = ["/Uploads"], + AllowedUploadDirectories = [this._tempDirectory], + AllowedUploadDestinationPaths = ["/Uploads"] + }; + + await InvokeRemoteOperationAsync(plugin, operation, suppliedPath, sourcePath); + + connector.VerifyAll(); + } + + [Theory] + [InlineData("read")] + [InlineData("share")] + [InlineData("upload")] + public async Task InvalidRemoteAllowListConfigurationRemainsVisibleAsync(string operation) + { + var connector = new Mock(MockBehavior.Strict); + var plugin = new CloudDrivePlugin(connector.Object) + { + AllowedReadPaths = ["/Uploads/%2e%2e"], + AllowedSharePaths = ["/Uploads/%2e%2e"], + AllowedUploadDestinationPaths = ["/Uploads/%2e%2e"] + }; + + var exception = await Assert.ThrowsAsync( + () => InvokeRemoteOperationAsync( + plugin, + operation, + "/Uploads/report.txt", + Path.Combine(this._tempDirectory, "source.txt"))); + + Assert.Contains("Configured allowed remote path", exception.Message, StringComparison.Ordinal); + connector.VerifyNoOtherCalls(); + } + + [Theory] + [InlineData("read")] + [InlineData("share")] + [InlineData("upload")] + public async Task RemoteOperationsRejectMatchingNamePrefixOutsideAllowedFolderAsync(string operation) + { + var connector = new Mock(MockBehavior.Strict); + var plugin = new CloudDrivePlugin(connector.Object) + { + AllowedReadPaths = ["/Uploads"], + AllowedSharePaths = ["/Uploads"], + AllowedUploadDestinationPaths = ["/Uploads"] + }; + + await Assert.ThrowsAsync( + () => InvokeRemoteOperationAsync( + plugin, + operation, + "/UploadsOther/report.txt", + Path.Combine(this._tempDirectory, "source.txt"))); + connector.VerifyNoOtherCalls(); + } + [SymbolicLinkTheory] [InlineData(false)] [InlineData(true)] @@ -751,4 +857,70 @@ public void Dispose() Directory.Delete(this._tempDirectory, recursive: true); GC.SuppressFinalize(this); } + + public static TheoryData DangerousRemotePaths + { + get + { + string[] paths = + [ + "/Uploads/%2e/Confidential/secret.txt", + "/Uploads/%2e%2e/Confidential/secret.txt", + "/Uploads/%2E%2E/Confidential/secret.txt", + "/Uploads/%2e./Confidential/secret.txt", + "/Uploads/.%2e/Confidential/secret.txt", + "/Uploads/%252e%252e/Confidential/secret.txt", + "/Uploads/%2e%2e%2fConfidential/secret.txt", + "/Uploads/%2e%2e%5cConfidential/secret.txt", + "/Uploads/%252f%252e%252e%252fConfidential/secret.txt" + ]; + TheoryData data = []; + foreach (var operation in new[] { "read", "share", "upload" }) + { + foreach (var path in paths) + { + data.Add(operation, path); + } + } + + return data; + } + } + + public static TheoryData AllowedNormalizedRemotePaths + { + get + { + TheoryData data = []; + foreach (var operation in new[] { "read", "share", "upload" }) + { + data.Add(operation, "/Uploads/Sub/../report.txt", "/Uploads/report.txt"); + data.Add(operation, "/Uploads/a%20b.txt", "/Uploads/a%20b.txt"); + } + + return data; + } + } + + private static async Task InvokeRemoteOperationAsync( + CloudDrivePlugin plugin, + string operation, + string remotePath, + string localPath) + { + switch (operation) + { + case "read": + await plugin.GetFileContentAsync(remotePath); + break; + case "share": + await plugin.CreateLinkAsync(remotePath); + break; + case "upload": + await plugin.UploadFileAsync(localPath, remotePath); + break; + default: + throw new InvalidOperationException($"Unknown test operation '{operation}'."); + } + } } diff --git a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/OneDriveConnectorTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/OneDriveConnectorTests.cs index 8689b1cb392c..0dfed8e273b2 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/OneDriveConnectorTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/MsGraph/OneDriveConnectorTests.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All rights reserved. +using System; using System.IO; using System.Net; using System.Net.Http; @@ -8,6 +9,7 @@ using System.Threading.Tasks; using Microsoft.Graph; using Microsoft.Kiota.Abstractions.Authentication; +using Microsoft.SemanticKernel.Plugins.MsGraph; using Microsoft.SemanticKernel.Plugins.MsGraph.Connectors; using Xunit; @@ -15,6 +17,99 @@ namespace SemanticKernel.Plugins.UnitTests.MsGraph; public sealed class OneDriveConnectorTests { + [Theory] + [InlineData("read", "/Uploads/Sub/../report.txt", "/Uploads/report.txt", ":/content")] + [InlineData("share", "/Uploads/Sub/../report.txt", "/Uploads/report.txt", ":/createLink")] + [InlineData("upload", "/Uploads/Sub/../report.txt", "/Uploads/report.txt", ":/content")] + [InlineData("read", "/Uploads/a%20b.txt", "/Uploads/a%20b.txt", ":/content")] + [InlineData("share", "/Uploads/a%20b.txt", "/Uploads/a%20b.txt", ":/createLink")] + [InlineData("upload", "/Uploads/a%20b.txt", "/Uploads/a%20b.txt", ":/content")] + public async Task PluginAuthorizationAndGraphRequestUseTheSameRemotePathAsync( + string operation, + string suppliedPath, + string expectedPath, + string requestSuffix) + { + using var handler = new GraphRequestHandler(); + using var client = new HttpClient(handler); + using var graph = new GraphServiceClient(client, new AnonymousAuthenticationProvider()); + var connector = new OneDriveConnector(graph); + var tempDirectory = Path.Combine(Path.GetTempPath(), $"OneDriveConnectorTests_{Guid.NewGuid():N}"); + Directory.CreateDirectory(tempDirectory); + var sourcePath = Path.Combine(tempDirectory, "source.txt"); + await File.WriteAllTextAsync(sourcePath, "content"); + try + { + var plugin = new CloudDrivePlugin(connector) + { + AllowedReadPaths = ["/Uploads"], + AllowedSharePaths = ["/Uploads"], + AllowedUploadDirectories = [tempDirectory], + AllowedUploadDestinationPaths = ["/Uploads"] + }; + + switch (operation) + { + case "read": + Assert.Equal("content", await plugin.GetFileContentAsync(suppliedPath)); + break; + case "share": + Assert.Equal("https://example.com/share", await plugin.CreateLinkAsync(suppliedPath)); + break; + case "upload": + await plugin.UploadFileAsync(sourcePath, suppliedPath); + break; + default: + throw new InvalidOperationException($"Unknown test operation '{operation}'."); + } + + Assert.NotNull(handler.OperationRequestUri); + Assert.Equal( + $"/v1.0/drives/test-drive/root:{expectedPath}{requestSuffix}", + handler.OperationRequestUri.AbsolutePath); + } + finally + { + Directory.Delete(tempDirectory, recursive: true); + } + } + + [Theory] + [InlineData("read")] + [InlineData("share")] + [InlineData("upload")] + public async Task EncodedTraversalIsRejectedBeforeAnyGraphRequestAsync(string operation) + { + using var handler = new GraphRequestHandler(); + using var client = new HttpClient(handler); + using var graph = new GraphServiceClient(client, new AnonymousAuthenticationProvider()); + var plugin = new CloudDrivePlugin(new OneDriveConnector(graph)) + { + AllowedReadPaths = ["/Uploads"], + AllowedSharePaths = ["/Uploads"], + AllowedUploadDestinationPaths = ["/Uploads"] + }; + const string Path = "/Uploads/%2e%2e/Confidential/secret.txt"; + + await Assert.ThrowsAsync(async () => + { + switch (operation) + { + case "read": + await plugin.GetFileContentAsync(Path); + break; + case "share": + await plugin.CreateLinkAsync(Path); + break; + case "upload": + await plugin.UploadFileAsync("unused.txt", Path); + break; + } + }); + + Assert.Equal(0, handler.RequestCount); + } + [Fact] public async Task UploadUsesProvidedStreamAndLeavesItOpenAsync() { @@ -66,4 +161,45 @@ protected override async Task SendAsync( }; } } + + private sealed class GraphRequestHandler : HttpMessageHandler + { + internal int RequestCount { get; private set; } + + internal Uri? OperationRequestUri { get; private set; } + + protected override async Task SendAsync( + HttpRequestMessage request, CancellationToken cancellationToken) + { + this.RequestCount++; + var path = request.RequestUri!.AbsolutePath; + if (path.EndsWith("/me/drive", StringComparison.Ordinal)) + { + return JsonResponse("{\"id\":\"test-drive\"}"); + } + + this.OperationRequestUri = request.RequestUri; + if (request.Method == HttpMethod.Put) + { + _ = await request.Content!.ReadAsByteArrayAsync(cancellationToken); + return JsonResponse("{\"id\":\"uploaded\"}"); + } + + if (request.Method == HttpMethod.Post) + { + return JsonResponse("{\"link\":{\"webUrl\":\"https://example.com/share\"}}"); + } + + return new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new StringContent("content", Encoding.UTF8, "text/plain") + }; + } + + private static HttpResponseMessage JsonResponse(string json) => + new(HttpStatusCode.OK) + { + Content = new StringContent(json, Encoding.UTF8, "application/json") + }; + } } From 4721002f520945f3bd497e263c6718d4f730876f Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Fri, 30 Oct 2026 12:57:37 +0000 Subject: [PATCH 6/7] Address PR comments --- .../CodeInterpreter/SessionsPythonPlugin.cs | 40 +++++-------------- .../Core/SessionsPythonPluginTests.cs | 9 +++-- 2 files changed, 15 insertions(+), 34 deletions(-) diff --git a/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs b/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs index 21a5f3b4951f..52c66933d6dc 100644 --- a/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs +++ b/dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs @@ -128,16 +128,16 @@ public async Task UploadFileAsync( Verify.NotNullOrWhiteSpace(localFilePath, nameof(localFilePath)); ValidateRemoteFileName(remoteFileName); - var validatedLocalPath = this.ValidateLocalPathForUpload(localFilePath); - - this._logger.LogInformation("Uploading file: {LocalFilePath} to {RemoteFileName}", validatedLocalPath, remoteFileName); - - using var httpClient = this._httpClientFactory.CreateClient(); + this.ValidateFileUploadConfiguration(); using var localFile = SecureFileSystem.OpenRead( localFilePath, this._settings.AllowedUploadDirectories!, - out _); + out var resolvedLocalPath); + + this._logger.LogInformation("Uploading file: {LocalFilePath} to {RemoteFileName}", resolvedLocalPath, remoteFileName); + + using var httpClient = this._httpClientFactory.CreateClient(); using var fileContent = new StreamContent(localFile); using var multipartFormDataContent = new MultipartFormDataContent() @@ -317,12 +317,10 @@ private static void ValidateRemoteFileName(string remoteFileName) } /// - /// Validates that the local file path is within allowed upload directories. + /// Validates that local file uploads are enabled and configured. /// - /// The local file path to validate. - /// The canonicalized path if valid. - /// Thrown when file operations are disabled or the path is not allowed. - private string ValidateLocalPathForUpload(string localFilePath) + /// Thrown when file uploads are disabled or allowed directories are not configured. + private void ValidateFileUploadConfiguration() { if (!this._settings.EnableDangerousFileUploads) { @@ -335,26 +333,6 @@ private string ValidateLocalPathForUpload(string localFilePath) throw new InvalidOperationException( "File upload requires 'AllowedUploadDirectories' to be configured."); } - - var canonicalPath = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(localFilePath)); - - foreach (var allowedDir in this._settings.AllowedUploadDirectories) - { - var canonicalAllowedDir = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(allowedDir)); - // Ensure we match the directory correctly by appending separator - var separator = Path.DirectorySeparatorChar.ToString(); - var allowedDirWithSeparator = canonicalAllowedDir.EndsWith(separator, PathUtilities.PathComparison) - ? canonicalAllowedDir - : canonicalAllowedDir + separator; - - if (canonicalPath.StartsWith(allowedDirWithSeparator, PathUtilities.PathComparison)) - { - return canonicalPath; - } - } - - throw new InvalidOperationException( - $"Access denied: '{localFilePath}' is not within allowed upload directories."); } /// diff --git a/dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs b/dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs index c633765f5d0d..9ea403f24312 100644 --- a/dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs +++ b/dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs @@ -562,7 +562,8 @@ public async Task ItShouldDenyUploadOutsideAllowedDirectoriesAsync() var exception = await Assert.ThrowsAsync( () => plugin.UploadFileAsync("test.txt", FileTestDataFilePath)); - Assert.Contains("not within allowed upload directories", exception.Message); + Assert.Contains("not within allowed directories", exception.Message); + Assert.Null(this._messageHandlerStub.RequestUri); } [Fact] @@ -771,7 +772,8 @@ public async Task ItShouldDenyUploadWithDifferentPathCasingOnLinuxAsync() var exception = await Assert.ThrowsAsync( () => plugin.UploadFileAsync("test.txt", disallowedFile)); - Assert.Contains("not within allowed upload directories", exception.Message); + Assert.Contains("not within allowed directories", exception.Message); + Assert.Null(this._messageHandlerStub.RequestUri); } finally { @@ -805,7 +807,8 @@ public async Task ItShouldDenyUploadWithPathTraversalAsync() var exception = await Assert.ThrowsAsync( () => plugin.UploadFileAsync("test.txt", traversalPath)); - Assert.Contains("not within allowed upload directories", exception.Message); + Assert.Contains("not within allowed directories", exception.Message); + Assert.Null(this._messageHandlerStub.RequestUri); } public void Dispose() From 273e8410f853a79a13615e057ca5ded7899149aa Mon Sep 17 00:00:00 2001 From: westey <164392973+westey-m@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:34:03 +0000 Subject: [PATCH 7/7] Address PR comments --- .../src/System/SecureFileSystem.cs | 99 ++++++++++--------- 1 file changed, 50 insertions(+), 49 deletions(-) diff --git a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs index 14f25d8c9182..2327339af163 100644 --- a/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs +++ b/dotnet/src/InternalUtilities/src/System/SecureFileSystem.cs @@ -62,7 +62,6 @@ internal static FileStream OpenUpdate( allowedPath, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None, rejectReadOnly: true, out created); } -#pragma warning disable CA2000 // The returned FileStream owns the handle; failures dispose it below. private static FileStream OpenRestrictedFile( AllowedPath allowedPath, FileMode mode, @@ -71,44 +70,45 @@ private static FileStream OpenRestrictedFile( bool rejectReadOnly, out bool created) { - SafeFileHandle fileHandle; - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) - { - fileHandle = WindowsFileSystem.Open( - allowedPath.AllowedDirectory, allowedPath.RelativeSegments, mode, access, share, rejectReadOnly, out created); - } - else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || - RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) - { - fileHandle = UnixFileSystem.Open( - allowedPath.AllowedDirectory, allowedPath.RelativeSegments, mode, access, share, rejectReadOnly, out created); - } - else - { - throw new PlatformNotSupportedException( - "Secure local file access is supported only on Windows, Linux, and macOS."); - } - + SafeFileHandle? fileHandle = null; FileStream? stream = null; try { + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + { + fileHandle = WindowsFileSystem.Open( + allowedPath.AllowedDirectory, allowedPath.RelativeSegments, mode, access, share, rejectReadOnly, out created); + } + else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || + RuntimeInformation.IsOSPlatform(OSPlatform.OSX)) + { + fileHandle = UnixFileSystem.Open( + allowedPath.AllowedDirectory, allowedPath.RelativeSegments, mode, access, share, rejectReadOnly, out created); + } + else + { + throw new PlatformNotSupportedException( + "Secure local file access is supported only on Windows, Linux, and macOS."); + } + stream = new FileStream(fileHandle, access); + fileHandle = null; if (mode == FileMode.Create) { // Do not truncate an existing file until the securely opened handle has passed every check. stream.SetLength(0); } - return stream; + var result = stream; + stream = null; + return result; } - catch + finally { stream?.Dispose(); - fileHandle.Dispose(); - throw; + fileHandle?.Dispose(); } } -#pragma warning restore CA2000 internal static string NormalizePath(string path, bool expandEnvironmentVariables = false) { @@ -223,7 +223,6 @@ private static class UnixFileSystem private const uint DefaultPermissions = 0x1B6; // 0666, reduced by the process umask. private const int AlreadyExists = 17; -#pragma warning disable CA2000 // Ownership of the verified file handle is returned; failed checks dispose it. internal static SafeFileHandle Open( string allowedDirectory, string[] relativeSegments, @@ -235,31 +234,36 @@ internal static SafeFileHandle Open( { created = false; var directoryFlags = GetDirectoryFlags(); - var currentHandle = OpenPath("/", directoryFlags); + SafeFileHandle? currentHandle = null; + SafeFileHandle? nextHandle = null; + SafeFileHandle? fileHandle = null; try { + currentHandle = OpenPath("/", directoryFlags); + // Start at the filesystem root and open each directory relative to its parent. // This keeps every parent stable even if its name is replaced during the walk. foreach (var segment in GetSegmentsAfterRoot(allowedDirectory)) { - var nextHandle = OpenAt(currentHandle, segment, directoryFlags, 0); + nextHandle = OpenAt(currentHandle, segment, directoryFlags, 0); currentHandle.Dispose(); currentHandle = nextHandle; + nextHandle = null; } for (var i = 0; i < relativeSegments.Length - 1; i++) { - var nextHandle = OpenAt(currentHandle, relativeSegments[i], directoryFlags, 0); + nextHandle = OpenAt(currentHandle, relativeSegments[i], directoryFlags, 0); currentHandle.Dispose(); currentHandle = nextHandle; + nextHandle = null; } var name = relativeSegments[relativeSegments.Length - 1]; var fileFlags = (access == FileAccess.Read ? ReadOnly : access == FileAccess.Write ? WriteOnly : ReadWrite) | GetNoFollowFlag() | GetCloseOnExecFlag() | GetNonBlockingFlag(); - SafeFileHandle handle; if (mode == FileMode.OpenOrCreate) { // Exclusive creation supplies reliable creation status. A conflicting file is @@ -268,12 +272,12 @@ internal static SafeFileHandle Open( fileFlags | GetCreateFlag() | GetExclusiveFlag(), DefaultPermissions); if (descriptor >= 0) { - handle = new SafeFileHandle(new IntPtr(descriptor), ownsHandle: true); + fileHandle = new SafeFileHandle(new IntPtr(descriptor), ownsHandle: true); created = true; } else if (Marshal.GetLastWin32Error() == AlreadyExists) { - handle = OpenAt(currentHandle, name, fileFlags, 0); + fileHandle = OpenAt(currentHandle, name, fileFlags, 0); } else { @@ -292,34 +296,29 @@ internal static SafeFileHandle Open( fileFlags |= GetExclusiveFlag(); } - handle = OpenAt(currentHandle, name, fileFlags, DefaultPermissions); + fileHandle = OpenAt(currentHandle, name, fileFlags, DefaultPermissions); created = mode == FileMode.CreateNew; } - try + // Nonblocking open prevents a substituted pipe from hanging before we can + // inspect the handle. Only ordinary files may reach the returned FileStream. + CheckFile(fileHandle, rejectReadOnly); + if (Flock(fileHandle, (share == FileShare.None ? 2 : 1) | 4) != 0) { - // Nonblocking open prevents a substituted pipe from hanging before we can - // inspect the handle. Only ordinary files may reach the returned FileStream. - CheckFile(handle, rejectReadOnly); - if (Flock(handle, (share == FileShare.None ? 2 : 1) | 4) != 0) - { - throw new IOException("The file is in use."); - } - - return handle; - } - catch - { - handle.Dispose(); - throw; + throw new IOException("The file is in use."); } + + var result = fileHandle; + fileHandle = null; + return result; } finally { - currentHandle.Dispose(); + fileHandle?.Dispose(); + nextHandle?.Dispose(); + currentHandle?.Dispose(); } } -#pragma warning restore CA2000 private static int GetDirectoryFlags() => // Traversal needs search permission, not permission to list each ancestor. @@ -387,6 +386,8 @@ private static void CheckFile(SafeFileHandle handle, bool rejectReadOnly) // statx has the same layout on all Linux architectures, unlike struct stat. if (StatX(handle, string.Empty, 0x1000, 0x3, out var information) != 0) { + // With SetLastError = true this returns the captured Unix errno despite its + // name; GetLastPInvokeError is unavailable on the netstandard2.0 target. throw new IOException($"Could not inspect the open file. Native error: {Marshal.GetLastWin32Error()}."); }