.Net: Add deny-by-default AllowedUploadDirectories to CloudDrivePlugin#13953
.Net: Add deny-by-default AllowedUploadDirectories to CloudDrivePlugin#13953SergeyMenshykh wants to merge 7 commits intomicrosoft:mainfrom
Conversation
- Add AllowedUploadDirectories property (defaults to empty = deny-all) - Validate local file path against allowlist before uploading - Canonicalize paths with env var expansion, UNC rejection, and traversal protection - Add unit tests for security behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
had a problem deploying
to
integration
GitHub Actions
Failure
github-actions
Bot
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 89%
✓ Correctness
This PR adds path-allowlisting security to CloudDrivePlugin's upload functionality. The implementation correctly canonicalizes paths (expanding environment variables and resolving traversal), validates against an explicit allowlist, blocks UNC paths, and permits subdirectories of allowed directories. The logic in IsUploadPathAllowed is sound — it properly handles trailing separators to prevent prefix attacks (e.g., /tmp/allowed vs /tmp/allowed-evil). Tests cover the key scenarios including default-deny, traversal, UNC, subdirectories, and environment variable expansion. No blocking correctness issues found.
✓ Security Reliability
This PR adds a security allowlist for upload directories in CloudDrivePlugin, implementing a deny-by-default pattern. The implementation correctly canonicalizes paths before validation (preventing traversal), expands environment variables before checking the allowlist, blocks UNC paths, and handles subdirectory matching with proper trailing-separator logic. The design is sound and well-tested. No blocking security or reliability issues identified.
✓ Test Coverage
The PR adds good security guardrails to CloudDrivePlugin with an AllowedUploadDirectories allowlist, and includes solid test coverage for the core security scenarios (default deny, path traversal, UNC paths, subdirectory access). However, there is a cross-platform fragility concern in the environment variable expansion test that relies on %TEMP% being set (Windows-only convention), and the existing success test mises an opportunity to verify that the canonicalized path (not the raw input) is forwarded to the connector.
✗ Design Approach
The new upload allowlist closes a real security gap, but the way it is introduced creates two design problems: it is a silent runtime-breaking change for existing CloudDrivePlugin consumers because the new policy is only configurable through an optional mutable property, and the path-matching logic hard-codes case-insensitive Windows semantics into a plugin that is exercised on Linux as well. Both issues point to the same root problem: the security policy needs to be modeled as an explicit, cross-platform contract rather than bolted onto UploadFileAsync as ad-hoc string checks.
Flagged Issues
- The allowlist uses OrdinalIgnoreCase path matching even though the repo runs .NET on Ubuntu (.github/workflows/dotnet-build-and-test.yml:59-67, .github/workflows/dotnet-ci.yml:22-25), so on Linux it can treat distinct directories (e.g., /tmp/Allowed vs /tmp/allowed) as equivalent, admitting files outside the intended allowlist.
Automated review by SergeyMenshykh's agents
There was a problem hiding this comment.
Pull request overview
This PR adds a deny-by-default local path allowlist (AllowedUploadDirectories) to CloudDrivePlugin so uploads are blocked unless explicitly configured, aiming to reduce the risk of unintended local file exfiltration when the plugin is exposed via auto function calling.
Changes:
- Introduces
AllowedUploadDirectories(default deny-all) and validates uploadfilePathagainst it after canonicalization. - Adds path canonicalization logic (env var expansion +
GetFullPath) and explicit UNC rejection. - Updates/extends unit tests to cover the new allowlist behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs | Adds allowlist property and enforces deny-by-default upload path validation. |
| dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs | Updates the existing upload test and adds new tests for deny/allow scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Use OrdinalIgnoreCase on Windows and Ordinal on Linux/macOS to respect case-sensitive file systems. Add test for platform-aware case sensitivity behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Validate filePath before canonicalization to provide a clear error. Include AllowedUploadDirectories remediation hint in deny message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
had a problem deploying
to
integration
GitHub Actions
Failure
…d filePath guard - Make AllowedUploadDirectories non-nullable; null assignments treated as empty (deny-all) - Add filePath null/whitespace guard before canonicalization - Include AllowedUploadDirectories hint in deny error message Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
had a problem deploying
to
integration
GitHub Actions
Failure
- Use specific ArgumentException assertion for UNC path test - Use dedicated test env var instead of platform-specific TEMP - Restore env var in finally blocks for test isolation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
had a problem deploying
to
integration
GitHub Actions
Failure
SergeyMenshykh
had a problem deploying
to
integration
GitHub Actions
Failure
On Linux, UNC paths are canonicalized differently and hit the allowlist denial (InvalidOperationException) instead of the UNC check (ArgumentException). Both correctly deny the upload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Add deny-by-default
AllowedUploadDirectoriestoCloudDrivePlugin.AllowedUploadDirectoriesproperty (defaults to empty = deny-all)UploadFileAsyncnow validates the local file path against the allowlist before uploading