.NET: Filestore improvements (#5842)

* Filestore improvements * Address PR comments
2026-06-16 21:04:09 +08:00 · 2026-05-14 12:09:01 +01:00
parent d40670748d
commit 4e65fabafc
2 changed files with 525 additions and 1 deletions
@@ -122,6 +122,7 @@ public sealed class FileSystemAgentFileStore : AgentFileStore
        }

        var files = Directory.GetFiles(fullDir)
+            .Where(f => (File.GetAttributes(f) & FileAttributes.ReparsePoint) == 0)
            .Select(Path.GetFileName)
            .Where(name => name is not null)
            .ToList();
@@ -157,6 +158,12 @@ public sealed class FileSystemAgentFileStore : AgentFileStore

        foreach (string filePath in Directory.GetFiles(fullDir))
        {
+            // Skip files that are symlinks/reparse points to prevent reading outside the root.
+            if ((File.GetAttributes(filePath) & FileAttributes.ReparsePoint) != 0)
+            {
+                continue;
+            }
+
            string? fileName = Path.GetFileName(filePath);
            if (fileName is null)
            {
@@ -231,7 +238,7 @@ public sealed class FileSystemAgentFileStore : AgentFileStore

    /// <summary>
    /// Resolves a relative file path to a safe absolute path under the root directory.
-    /// Rejects paths that would escape the root via traversal or rooted paths.
+    /// Rejects paths that would escape the root via traversal, rooted paths, or symbolic links.
    /// </summary>
    private string ResolveSafePath(string relativePath)
    {
@@ -250,9 +257,55 @@ public sealed class FileSystemAgentFileStore : AgentFileStore
                nameof(relativePath));
        }

+        // Reject symlinks/reparse points in any path segment to prevent escaping the root.
+        ThrowIfContainsSymlink(fullPath, this._rootPath);
+
        return fullPath;
    }

+    /// <summary>
+    /// Checks each path segment between the trusted root and the resolved path for symbolic links
+    /// or reparse points. Throws <see cref="ArgumentException"/> if any segment is a symlink.
+    /// Stops checking at the first segment that does not exist on disk (for write scenarios).
+    /// Uses <see cref="File.GetAttributes(string)"/> directly so that dangling symlinks (whose targets
+    /// do not exist) are still detected via their <see cref="FileAttributes.ReparsePoint"/> flag.
+    /// </summary>
+    private static void ThrowIfContainsSymlink(string fullPath, string rootPath)
+    {
+        string rootTrimmed = rootPath.TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar);
+        string relative = fullPath.Substring(rootTrimmed.Length);
+        string[] segments = relative.Split(
+            [Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar],
+            StringSplitOptions.RemoveEmptyEntries);
+
+        string current = rootTrimmed;
+        foreach (string segment in segments)
+        {
+            current = Path.Combine(current, segment);
+
+            FileAttributes attributes;
+            try
+            {
+                attributes = File.GetAttributes(current);
+            }
+            catch (FileNotFoundException)
+            {
+                // Segment does not exist on disk (write scenario); stop checking.
+                break;
+            }
+            catch (DirectoryNotFoundException)
+            {
+                break;
+            }
+
+            if ((attributes & FileAttributes.ReparsePoint) != 0)
+            {
+                throw new ArgumentException(
+                    "Invalid path: the resolved path contains a symbolic link or reparse point.");
+            }
+        }
+    }
+
    /// <summary>
    /// Resolves a relative directory path to a safe absolute path under the root directory.
    /// An empty string resolves to the root directory itself.
@@ -334,4 +334,475 @@ public sealed class FileSystemAgentFileStoreTests : IDisposable
    }

    #endregion
+
+    #region Symlink Escape Rejection
+
+#if NET
+    /// <summary>
+    /// Attempts to create a file symlink. Returns false if the platform does not support
+    /// symlink creation (e.g., Windows without developer mode) or if creation fails.
+    /// </summary>
+    private static bool TryCreateFileSymbolicLink(string linkPath, string targetPath)
+    {
+        try
+        {
+            File.CreateSymbolicLink(linkPath, targetPath);
+        }
+        catch (IOException)
+        {
+            return false;
+        }
+
+        // Verify the symlink was actually created as a reparse point.
+        return File.Exists(linkPath)
+            && (File.GetAttributes(linkPath) & FileAttributes.ReparsePoint) != 0;
+    }
+
+    /// <summary>
+    /// Attempts to create a directory symlink. Returns false if the platform does not support
+    /// symlink creation (e.g., Windows without developer mode) or if creation fails.
+    /// </summary>
+    private static bool TryCreateDirectorySymbolicLink(string linkPath, string targetPath)
+    {
+        try
+        {
+            Directory.CreateSymbolicLink(linkPath, targetPath);
+        }
+        catch (IOException)
+        {
+            return false;
+        }
+
+        // Verify the symlink was actually created as a reparse point.
+        return Directory.Exists(linkPath)
+            && (File.GetAttributes(linkPath) & FileAttributes.ReparsePoint) != 0;
+    }
+
+    [Fact]
+    public async Task ReadFileAsync_SymlinkedFile_ThrowsAsync()
+    {
+        // Arrange — create a file outside the root and symlink to it from inside.
+        string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_target_read_" + Guid.NewGuid().ToString("N") + ".txt");
+        File.WriteAllText(outsideFile, "SECRET_OUTSIDE_ROOT");
+
+        string linkPath = Path.Combine(this._rootDir, "leak.txt");
+
+        try
+        {
+            if (!TryCreateFileSymbolicLink(linkPath, outsideFile))
+            {
+                return; // Cannot create symlinks in this environment; skip.
+            }
+
+            // Act & Assert — reading through the symlink should be rejected.
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.ReadFileAsync("leak.txt"));
+        }
+        finally
+        {
+            if (File.Exists(linkPath))
+            {
+                File.Delete(linkPath);
+            }
+
+            File.Delete(outsideFile);
+        }
+    }
+
+    [Fact]
+    public async Task WriteFileAsync_SymlinkedFile_ThrowsAsync()
+    {
+        // Arrange — create a file outside the root and symlink to it from inside.
+        string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_target_write_" + Guid.NewGuid().ToString("N") + ".txt");
+        File.WriteAllText(outsideFile, "ORIGINAL_CONTENT");
+
+        string linkPath = Path.Combine(this._rootDir, "overwrite.txt");
+
+        try
+        {
+            if (!TryCreateFileSymbolicLink(linkPath, outsideFile))
+            {
+                return;
+            }
+
+            // Act & Assert — writing through the symlink should be rejected.
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.WriteFileAsync("overwrite.txt", "EVIL_CONTENT"));
+
+            // Verify the outside file was NOT modified.
+            Assert.Equal("ORIGINAL_CONTENT", await File.ReadAllTextAsync(outsideFile));
+        }
+        finally
+        {
+            if (File.Exists(linkPath))
+            {
+                File.Delete(linkPath);
+            }
+
+            File.Delete(outsideFile);
+        }
+    }
+
+    [Fact]
+    public async Task DeleteFileAsync_SymlinkedFile_ThrowsAsync()
+    {
+        // Arrange
+        string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_target_delete_" + Guid.NewGuid().ToString("N") + ".txt");
+        File.WriteAllText(outsideFile, "DO_NOT_DELETE");
+
+        string linkPath = Path.Combine(this._rootDir, "trap.txt");
+
+        try
+        {
+            if (!TryCreateFileSymbolicLink(linkPath, outsideFile))
+            {
+                return;
+            }
+
+            // Act & Assert
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.DeleteFileAsync("trap.txt"));
+
+            // Verify the outside file still exists.
+            Assert.True(File.Exists(outsideFile));
+        }
+        finally
+        {
+            if (File.Exists(linkPath))
+            {
+                File.Delete(linkPath);
+            }
+
+            File.Delete(outsideFile);
+        }
+    }
+
+    [Fact]
+    public async Task FileExistsAsync_SymlinkedFile_ThrowsAsync()
+    {
+        // Arrange
+        string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_target_exists_" + Guid.NewGuid().ToString("N") + ".txt");
+        File.WriteAllText(outsideFile, "EXISTS_OUTSIDE");
+
+        string linkPath = Path.Combine(this._rootDir, "phantom.txt");
+
+        try
+        {
+            if (!TryCreateFileSymbolicLink(linkPath, outsideFile))
+            {
+                return;
+            }
+
+            // Act & Assert
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.FileExistsAsync("phantom.txt"));
+        }
+        finally
+        {
+            if (File.Exists(linkPath))
+            {
+                File.Delete(linkPath);
+            }
+
+            File.Delete(outsideFile);
+        }
+    }
+
+    [Fact]
+    public async Task WriteFileAsync_DanglingSymlink_ThrowsAsync()
+    {
+        // Arrange — create a symlink pointing to a non-existent target.
+        string nonExistentTarget = Path.Combine(Path.GetTempPath(), "dangling_target_" + Guid.NewGuid().ToString("N") + ".txt");
+        string linkPath = Path.Combine(this._rootDir, "dangling.txt");
+
+        try
+        {
+            if (!TryCreateFileSymbolicLink(linkPath, nonExistentTarget))
+            {
+                return;
+            }
+
+            // Act & Assert — even a dangling symlink must be rejected.
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.WriteFileAsync("dangling.txt", "CONTENT"));
+
+            // Verify the target was NOT created by following the dangling link.
+            Assert.False(File.Exists(nonExistentTarget));
+        }
+        finally
+        {
+            // Dangling symlinks: File.Exists returns false, but the link entry still exists.
+            // Use FileInfo to delete the link itself.
+            var linkInfo = new FileInfo(linkPath);
+            if (linkInfo.Exists || (linkInfo.Attributes & FileAttributes.ReparsePoint) != 0)
+            {
+                linkInfo.Delete();
+            }
+        }
+    }
+
+    [Fact]
+    public async Task ListFilesAsync_SymlinkedDirectory_ThrowsAsync()
+    {
+        // Arrange — create a directory outside root and symlink a directory inside root to it.
+        string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_target_" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(outsideDir);
+        File.WriteAllText(Path.Combine(outsideDir, "secret.txt"), "SECRET");
+
+        string linkDir = Path.Combine(this._rootDir, "linked-dir");
+
+        try
+        {
+            if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir))
+            {
+                return;
+            }
+
+            // Act & Assert
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.ListFilesAsync("linked-dir"));
+        }
+        finally
+        {
+            if (Directory.Exists(linkDir))
+            {
+                Directory.Delete(linkDir);
+            }
+
+            Directory.Delete(outsideDir, recursive: true);
+        }
+    }
+
+    [Fact]
+    public async Task SearchFilesAsync_SymlinkedDirectory_ThrowsAsync()
+    {
+        // Arrange
+        string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_search_target_" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(outsideDir);
+        File.WriteAllText(Path.Combine(outsideDir, "data.txt"), "SENSITIVE_DATA");
+
+        string linkDir = Path.Combine(this._rootDir, "search-link");
+
+        try
+        {
+            if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir))
+            {
+                return;
+            }
+
+            // Act & Assert
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.SearchFilesAsync("search-link", "SENSITIVE"));
+        }
+        finally
+        {
+            if (Directory.Exists(linkDir))
+            {
+                Directory.Delete(linkDir);
+            }
+
+            Directory.Delete(outsideDir, recursive: true);
+        }
+    }
+
+    [Fact]
+    public async Task ReadFileAsync_ThroughDirectorySymlink_ThrowsAsync()
+    {
+        // Arrange — directory symlink inside root pointing outside; read a file through it.
+        string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_read_" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(outsideDir);
+        File.WriteAllText(Path.Combine(outsideDir, "secret.txt"), "DIR_SYMLINK_SECRET");
+
+        string linkDir = Path.Combine(this._rootDir, "linked-output");
+
+        try
+        {
+            if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir))
+            {
+                return;
+            }
+
+            // Act & Assert — reading through a directory symlink should be rejected.
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.ReadFileAsync("linked-output/secret.txt"));
+        }
+        finally
+        {
+            if (Directory.Exists(linkDir))
+            {
+                Directory.Delete(linkDir);
+            }
+
+            Directory.Delete(outsideDir, recursive: true);
+        }
+    }
+
+    [Fact]
+    public async Task WriteFileAsync_ThroughDirectorySymlink_ThrowsAsync()
+    {
+        // Arrange — directory symlink; attempt to create/overwrite a file through it.
+        string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_write_" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(outsideDir);
+
+        string linkDir = Path.Combine(this._rootDir, "linked-output");
+
+        try
+        {
+            if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir))
+            {
+                return;
+            }
+
+            // Act & Assert
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.WriteFileAsync("linked-output/created-by-agent.txt", "CONTENT"));
+
+            // Verify no file was created outside.
+            Assert.False(File.Exists(Path.Combine(outsideDir, "created-by-agent.txt")));
+        }
+        finally
+        {
+            if (Directory.Exists(linkDir))
+            {
+                Directory.Delete(linkDir);
+            }
+
+            Directory.Delete(outsideDir, recursive: true);
+        }
+    }
+
+    [Fact]
+    public async Task DeleteFileAsync_ThroughDirectorySymlink_ThrowsAsync()
+    {
+        // Arrange — directory symlink; attempt to delete a file through it.
+        string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_delete_" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(outsideDir);
+        string outsideFile = Path.Combine(outsideDir, "delete-me.txt");
+        File.WriteAllText(outsideFile, "DO_NOT_DELETE");
+
+        string linkDir = Path.Combine(this._rootDir, "linked-output");
+
+        try
+        {
+            if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir))
+            {
+                return;
+            }
+
+            // Act & Assert
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.DeleteFileAsync("linked-output/delete-me.txt"));
+
+            // Verify the outside file was NOT deleted.
+            Assert.True(File.Exists(outsideFile));
+        }
+        finally
+        {
+            if (Directory.Exists(linkDir))
+            {
+                Directory.Delete(linkDir);
+            }
+
+            Directory.Delete(outsideDir, recursive: true);
+        }
+    }
+
+    [Fact]
+    public async Task CreateDirectoryAsync_ThroughDirectorySymlink_ThrowsAsync()
+    {
+        // Arrange — directory symlink; attempt to create a subdirectory through it.
+        string outsideDir = Path.Combine(Path.GetTempPath(), "symlink_dir_mkdir_" + Guid.NewGuid().ToString("N"));
+        Directory.CreateDirectory(outsideDir);
+
+        string linkDir = Path.Combine(this._rootDir, "linked-output");
+
+        try
+        {
+            if (!TryCreateDirectorySymbolicLink(linkDir, outsideDir))
+            {
+                return;
+            }
+
+            // Act & Assert
+            await Assert.ThrowsAsync<ArgumentException>(() => this._store.CreateDirectoryAsync("linked-output/created-directory"));
+
+            // Verify no directory was created outside.
+            Assert.False(Directory.Exists(Path.Combine(outsideDir, "created-directory")));
+        }
+        finally
+        {
+            if (Directory.Exists(linkDir))
+            {
+                Directory.Delete(linkDir);
+            }
+
+            Directory.Delete(outsideDir, recursive: true);
+        }
+    }
+
+    [Fact]
+    public async Task SearchFilesAsync_RootWithSymlinkedFile_DoesNotLeakContentAsync()
+    {
+        // Arrange — symlinked file at root level; search should not return its content.
+        string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_search_root_" + Guid.NewGuid().ToString("N") + ".txt");
+        File.WriteAllText(outsideFile, "ROOT_LEVEL_SECRET_CONTENT");
+
+        string linkPath = Path.Combine(this._rootDir, "env-link.txt");
+
+        try
+        {
+            if (!TryCreateFileSymbolicLink(linkPath, outsideFile))
+            {
+                return;
+            }
+
+            // Also add a normal file to confirm search still works for non-symlinks.
+            await this._store.WriteFileAsync("normal.txt", "NORMAL_CONTENT");
+
+            // Act — search at root should skip the symlinked file.
+            var results = await this._store.SearchFilesAsync("", "SECRET_CONTENT");
+
+            // Assert — no results from the symlinked file.
+            Assert.Empty(results);
+        }
+        finally
+        {
+            if (File.Exists(linkPath))
+            {
+                File.Delete(linkPath);
+            }
+
+            File.Delete(outsideFile);
+        }
+    }
+
+    [Fact]
+    public async Task ListFilesAsync_RootWithSymlinkedFile_ExcludesSymlinkAsync()
+    {
+        // Arrange — symlinked file at root level; listing should not include it.
+        string outsideFile = Path.Combine(Path.GetTempPath(), "symlink_list_root_" + Guid.NewGuid().ToString("N") + ".txt");
+        File.WriteAllText(outsideFile, "OUTSIDE");
+
+        string linkPath = Path.Combine(this._rootDir, "hidden-link.txt");
+
+        try
+        {
+            if (!TryCreateFileSymbolicLink(linkPath, outsideFile))
+            {
+                return;
+            }
+
+            // Also add a normal file.
+            await this._store.WriteFileAsync("visible.txt", "VISIBLE");
+
+            // Act
+            var files = await this._store.ListFilesAsync("");
+
+            // Assert — symlinked file should not appear in listing.
+            Assert.DoesNotContain("hidden-link.txt", files);
+            Assert.Contains("visible.txt", files);
+        }
+        finally
+        {
+            if (File.Exists(linkPath))
+            {
+                File.Delete(linkPath);
+            }
+
+            File.Delete(outsideFile);
+        }
+    }
+#endif
+
+    #endregion
 }