Conversation
Accept the resolved AbsolutePath.Value property as an absolute-path source. Resolve the trusted framework Path once per analyzer compilation, independent of return type, and pass its symbol through canonicalization and recursive safe-path checks. Resolve once before the code-fix argument scan as well. Preserve original comments and document the safe pattern concisely. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Cover direct and transitive file calls, local initializers, unrelated or transformed values, and source/metadata Path lookalikes. Include nested Path calls and initializer chains. All snippets are compiler-valid and the complete analyzer suite passes 384 cases. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The typed-parameter analyzer still trusts display-name Path lookalikes in two branches.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates MSBuild task analyzers to recognize AbsolutePath.Value as safe and validate framework System.IO.Path identity.
Changes:
- Added
AbsolutePath.Valuesafety recognition and local-flow support. - Propagated framework
Pathsymbols through analyzers and code fixes. - Added regression tests and README documentation.
File summaries
| File | Description |
|---|---|
src/TaskAnalyzer/SharedAnalyzerHelpers.cs |
Implements value and framework-Path recognition. |
src/TaskAnalyzer/MultiThreadableTaskAnalyzer.cs |
Applies safety checks to direct analysis. |
src/TaskAnalyzer/TransitiveCallChainAnalyzer.cs |
Applies safety checks to transitive analysis. |
src/TaskAnalyzer/PreferTypedParameterAnalyzer.cs |
Threads path identity into typed-parameter analysis. |
src/TaskAnalyzer/MultiThreadableTaskCodeFixProvider.cs |
Uses updated safety checks for fixes. |
src/TaskAnalyzer/README.md |
Documents AbsolutePath.Value. |
src/TaskAnalyzer.Tests/MultiThreadableTaskAnalyzerTests.cs |
Adds direct and lookalike-path coverage. |
src/TaskAnalyzer.Tests/TransitiveCallChainAnalyzerTests.cs |
Adds transitive-flow coverage. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
| INamedTypeSymbol? directoryInfoType, | ||
| INamedTypeSymbol? systemIOPathType) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
| # | Dimension | Verdict |
|---|---|---|
| 11 | Cross-Platform Correctness | 🟡 1 MAJOR |
| 17 | File I/O & Path Handling | 🟡 1 MAJOR |
| 22 | Correctness & Edge Cases | 🟡 1 MAJOR |
✅ 21/24 dimensions clean.
- Cross-Platform Correctness — resolve framework
System.IO.Pathacross split contract assemblies - File I/O & Path Handling — prevent drive-relative
Path.Combinecomponents from being treated as absolute - Correctness & Edge Cases — account for local reassignment before propagating
AbsolutePath.Valuesafety
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
api.github.comgithub.comlearn.microsoft.comraw.githubusercontent.com
[!TIP]
api.github.com is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding api.github.com to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to api.github.com:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "api.github.com"
- "github.com"
- "learn.microsoft.com"
- "raw.githubusercontent.com"See Network Configuration for more information.
Generated by Expert Code Review (on open) for #15073 · copilot · gpt56 · 837 AIC · ⌖ 4.57 AIC · ⊞ 17.8K
| { | ||
| // Resolve Path from the core library, not a source or referenced lookalike. | ||
| var coreLibrary = compilation.GetSpecialType(SpecialType.System_Object).ContainingAssembly; | ||
| return coreLibrary?.GetTypeByMetadataName("System.IO.Path"); |
There was a problem hiding this comment.
[MAJOR] Cross-Platform Correctness
A contract-reference compilation can define System.Object in System.Runtime while System.IO.Path is defined in or forwarded through System.Runtime.Extensions. In that case coreLibrary.GetTypeByMetadataName("System.IO.Path") returns null, so every real Path.GetDirectoryName/Combine/GetFullPath call fails IsSystemIOPath and valid absolute-path code starts producing MSBuildTask0003 (including warnings-as-errors builds). The current tests use runtime implementation assemblies, where both types happen to resolve from System.Private.CoreLib, so they do not cover this layout.
Recommendation: resolve System.IO.Path from referenced assembly symbols (excluding the source assembly/lookalikes), deduplicate forwarded symbols with SymbolEqualityComparer.Default, and require one framework match. Add a Roslyn compilation test with split contract references (System.Runtime + System.Runtime.Extensions).
| IsSystemIOPath(invocation, systemIOPathType) && | ||
| invocation.Arguments.Length >= 2 && | ||
| IsWrappedSafely(invocation.Arguments[0].Value, taskEnvironmentType, absolutePathType, iTaskItemType)) | ||
| IsWrappedSafely(invocation.Arguments[0].Value, taskEnvironmentType, absolutePathType, iTaskItemType, systemIOPathType)) |
There was a problem hiding this comment.
[MAJOR] File I/O & Path Handling
This now treats Path.Combine(path.Value, @"C:relative.txt") as safe. On Windows the second component is drive-rooted but not fully qualified, and Path.Combine(@"D:\\safe", @"C:relative.txt") yields C:relative.txt; the file API then resolves it against drive C's current directory. Before this PR, AbsolutePath.Value did not satisfy the recursive check, so MSBuildTask0003 was retained.
AbsolutePath path = TaskEnvironment.GetAbsolutePath(@"D:\\safe");
File.OpenRead(Path.Combine(path.Value, @"C:relative.txt"));Recommendation: do not propagate safety from the first Combine argument unless every later component is proven unable to reset the root; at minimum reject rooted/drive-relative constant components. Add this Windows regression case.
| } | ||
|
|
||
| // AbsolutePath.Value is already fully qualified; no canonicalization is required. | ||
| if (IsAbsolutePathValue(operation, absolutePathType)) |
There was a problem hiding this comment.
[MAJOR] Correctness & Edge Cases
Marking AbsolutePath.Value safe exposes the existing initializer-only local tracing to stale values. IsWrappedSafely follows the declaration initializer but never checks later assignments, so this concrete relative access is now incorrectly accepted:
AbsolutePath absolutePath = TaskEnvironment.GetAbsolutePath("safe.txt");
string path = absolutePath.Value;
path = "relative.txt";
File.OpenRead(path); // MSBuildTask0003 is suppressedThe base behavior reported this because .Value was not considered safe.
Recommendation: only propagate safety through a local when data-flow/reaching-definition analysis proves the initializer still reaches the use (or conservatively stop propagation when the local has another write), and add this reassignment regression test.
| public TaskEnvironment TaskEnvironment { get; set; } | ||
| public override bool Execute() | ||
| { | ||
| new FileInfo(TaskEnvironment.GetAbsolutePath("foo.txt").Value); |
There was a problem hiding this comment.
I can see how this pattern isn't wrong but is it one we want to encourage? Isn't avoiding the need for this why we did the silent cast-to-string support?
There was a problem hiding this comment.
yes, I also was not sure whether we would like to encourage this. On the other hand, we, while already had the silent cast - i believe it was there from the first version, used Value ourselves and I think customers would do that as well. And it is not too wrong to explicitly do.
@baronfel what do you think?
Fixes #15072
Context
Passing
AbsolutePath.Valueto file APIs incorrectly producesMSBuildTask0003or transitiveMSBuildTask0005, despite the value already being absolute.Changes Made
Valueproperty of the resolvedAbsolutePathtype, including through existing local-initializer tracing.Pathhelpers so same-named lookalikes cannot qualify. Resolve the symbol once per analyzer compilation.Compatibility
No changes to analyzer activation or diagnostic severities. Existing canonicalization behavior is preserved; unrelated
Valueproperties and lookalikePathhelpers remain untrusted.