.Net: Harden CloudDrivePlugin defaults and add path validation#13958
Conversation
…th validation - Change CreateLinkAsync default scope from 'anonymous' to 'organization' - Add AllowedSharePaths property with deny-all default for share link operations - Add IsSharePathAllowed validation with normalized prefix matching - Update ICloudDriveConnector default scope parameter to 'organization' - Add regression tests for scope, deny-all default, path allow/deny, subdirectories Fixes: #115112, #115113 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.gh.lumenfield.work/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 88%
✓ Correctness
The PR adds path validation for share-link operations and hardens defaults. The
IsSharePathAllowedmethod has a path traversal vulnerability: it only normalizes backslashes to forward slashes but does not collapse..segments. An input like/Documents/../Confidential/secret.docxwithAllowedSharePaths = ["/Documents"]would pass theStartsWithcheck because the extracted directory/Documents/../Confidential/starts with/Documents/, yet the Graph API would resolve it to/Confidential/secret.docx. The local-path equivalent (IsUploadPathAllowed) is protected becauseCanonicalizePathcallsPath.GetFullPathwhich resolves traversal, but no analogous normalization exists for remote paths.
✓ Security Reliability
The PR adds path validation for share-link operations and hardens defaults (scope changed from 'anonymous' to 'organization'). However, the new
IsSharePathAllowedmethod has a path traversal vulnerability: it normalizes slashes but does not resolve..segments, so a path like/Documents/../Confidential/secret.docxwould pass validation when only/Documentsis allowed, since the string"/Documents/../Confidential/"starts with"/Documents/". TheIsUploadPathAllowedpath correctly usesPath.GetFullPath()for canonicalization, but the remote-path validator has no equivalent normalization for dot-dot sequences.
✓ Test Coverage
The PR adds good test coverage for the new AllowedSharePaths validation and CanonicalizePath refactoring. Tests cover happy path, deny-by-default, deny outside allowed paths, subdirectory allowance, organization scope verification, UNC-after-env-var-expansion, and traversal-after-env-var-expansion. One notable gap: no test verifies that path traversal segments ('..') in remote share paths are handled by IsSharePathAllowed, which uses only string prefix matching without resolving relative segments.
✗ Design Approach
I found one design-level issue worth requesting changes on: the PR hardens
CloudDrivePlugin.CreateLinkAsynccorrectly by passing"organization"explicitly, but it also changes the publicICloudDriveConnectoroptional default without changing the concreteOneDriveConnectormethod default. In C#, optional-parameter defaults are chosen from the static type at the callsite, so this creates two different default behaviors for the same operation depending on whether callers hold the object asICloudDriveConnectororOneDriveConnector.
Automated review by SergeyMenshykh's agents
There was a problem hiding this comment.
Pull request overview
This PR hardens the .NET MsGraph CloudDrivePlugin by making sharing behavior more restrictive by default, adding allowlist-based validation for share-link creation, and improving local upload path canonicalization (including env-var expansion) with regression tests.
Changes:
- Add
AllowedSharePathsallowlist and enforce it inCreateLinkAsync(deny by default). - Change share-link scope defaults to
"organization"and add tests covering the new defaults and validation behavior. - Centralize local path canonicalization for uploads (expand env vars, block UNC) and add env-var bypass regression tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| dotnet/src/Plugins/Plugins.UnitTests/MsGraph/CloudDrivePluginTests.cs | Adds/updates tests for share-path allowlisting and env-var expansion hardening. |
| dotnet/src/Plugins/Plugins.MsGraph/ICloudDriveConnector.cs | Changes default scope for share links to "organization". |
| dotnet/src/Plugins/Plugins.MsGraph/CloudDrivePlugin.cs | Introduces AllowedSharePaths, enforces share-path validation, and adds canonicalization for upload paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Collapse '..' segments in IsSharePathAllowed via NormalizeRemotePath() to prevent traversal bypass (e.g. /Documents/../Confidential/secret.docx) - Update OneDriveConnector.CreateShareLinkAsync default scope from 'anonymous' to 'organization' to match ICloudDriveConnector interface - Reject forward-slash UNC paths (//server/share) in CanonicalizePath, both before and after environment variable expansion - Clarify AllowedSharePaths XML docs: entries are directory prefixes - Add tests for share path traversal and forward-slash UNC rejection Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
had a problem deploying
to
integration
GitHub Actions
Failure
The Split(char, StringSplitOptions) overload is not available in .NET Standard 2.0. Use Split(char[], StringSplitOptions) instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…destination - Add AllowedReadPaths property to gate GetFileContentAsync (remote read) - Add AllowedUploadDestinationPaths property to gate UploadFileAsync destination - Refactor IsSharePathAllowed into reusable IsAllowedRemotePath helper - Update class-level docs to reference all four allow-list properties - Add regression tests for new validation (deny-by-default, path traversal, subdirectory support, connector-not-called assertions) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…destination - Add AllowedReadPaths property to gate GetFileContentAsync (remote read) - Add AllowedUploadDestinationPaths property to gate UploadFileAsync destination - Refactor IsSharePathAllowed into reusable IsAllowedRemotePath helper - Update class-level docs to reference all four allow-list properties - Add regression tests for new validation (deny-by-default, path traversal, subdirectory support, connector-not-called assertions) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…enshykh/semantic-kernel into fix/cloud-drive-security
…enshykh/semantic-kernel into fix/cloud-drive-security
…enshykh/semantic-kernel into fix/cloud-drive-security
lokitoth
left a comment
lokitoth
left a comment
There was a problem hiding this comment.
We have several places in the codebase where we do this some kind of path normalization / no-escape validation. Should we unify them into some shared set of helpers?
Let me try to extract common normalization and validation bits into sharable utils in the next PR. |
Description
Contribution Checklist