.Net: Fix DocumentPlugin path validation order

[SergeyMenshykh]

Motivation and Context

Fix path validation order in DocumentPlugin to canonicalize paths before validating against AllowedDirectories. Also removes redundant path transformation from LocalFileSystemConnector.

Description

• Canonicalize file paths (expand environment variables and resolve to absolute form) before validating against AllowedDirectories in both ReadTextAsync and AppendTextAsync
• Remove environment variable expansion from LocalFileSystemConnector the connector should operate on the path it receives, not transform it
• Add regression tests

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the SK Contribution Guidelines
• The existing tests pass
• New tests added

[Copilot]

Pull request overview

This PR hardens DocumentPlugin path handling by canonicalizing (env-var expansion + absolute resolution) before validating against AllowedDirectories, and aligns the local file system connector to operate strictly on provided paths. It also adds regression tests intended to prevent validation bypasses.

Changes:

• Canonicalize input paths before AllowedDirectories validation in ReadTextAsync and AppendTextAsync.
• Stop expanding environment variables inside LocalFileSystemConnector (plugin now provides canonical paths).
• Add/adjust unit tests for relative paths and env-var-based bypass attempts.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs	Canonicalizes file paths before allow-list validation and uses canonical paths for I/O/logging.
dotnet/src/Plugins/Plugins.Document/FileSystem/LocalFileSystemConnector.cs	Removes env-var expansion so the connector uses the provided path verbatim.
dotnet/src/Plugins/Plugins.UnitTests/Document/DocumentPluginTests.cs	Adds/updates regression tests for relative-path and env-var expansion bypass scenarios.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Automated Code Review

Reviewers: 4 | Confidence: 83%

✓ Correctness

The PR correctly fixes the validate/use mismatch by canonicalizing paths before validation. The core logic is sound: CanonicalizePath expands env vars and resolves to an absolute path, which is then passed to both IsFilePathAllowed and the file system connector. One defense-in-depth weakness: the UNC path check validates the raw input before environment variable expansion, so a path like %VAR%\file.txt (where VAR=\server\share) bypasses the UNC check. The allowed-directory check still prevents unauthorized access, but the explicit UNC ban becomes inconsistently enforced.

✓ Security Reliability

The PR correctly fixes the validate/use mismatch by canonicalizing paths before the AllowedDirectories check. However, the UNC path rejection check in CanonicalizePath is performed on the raw input path BEFORE environment variable expansion. An attacker could supply a path containing an env var that expands to a UNC prefix (e.g., %MYVAR%\file.txt where MYVAR=\\server\share), bypassing the UNC guard entirely. The fix should also verify the expanded/canonical result is not a UNC path.

✓ Test Coverage

The new regression tests correctly cover the security fix (env-var expansion bypass on both read/write paths). However, there are two notable test coverage gaps: (1) no positive-case test verifying that a path containing environment variables resolves correctly and is ALLOWED when it falls within an allowed directory, and (2) ItDeniesPercentEncodedPathTraversalOnReadAsync has no corresponding Write-path counterpart, unlike the other bypass tests which cover both operations.

✗ Design Approach

The path canonicalization change addresses the env-var ordering bug, but it still keeps Windows-style case-insensitive directory matching in DocumentPlugin. That leaves a cross-platform authorization gap on Linux/macOS that the repo already solves elsewhere with an OS-specific path comparison.

Flagged Issues

• DocumentPlugin still authorizes paths with StringComparison.OrdinalIgnoreCase, so on Linux/macOS an allowed directory like /tmp/Allowed would also permit /tmp/allowed/... (dotnet/src/Plugins/Plugins.Document/DocumentPlugin.cs:186-187). The repo already uses an OS-aware comparison for this exact problem in dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs:141-145 and :179-185.

---

Automated review by SergeyMenshykh's agents