Preserve whitespace-path throw in StaticWebAsset.NormalizeContentRootPath#54868
Open
jankratochvilcz wants to merge 2 commits into
Open
Preserve whitespace-path throw in StaticWebAsset.NormalizeContentRootPath#54868jankratochvilcz wants to merge 2 commits into
jankratochvilcz wants to merge 2 commits into
Conversation
After the multi-threaded migration, NormalizeContentRootPath(path, env) routed whitespace-only paths through TaskEnvironment.GetAbsolutePath, which silently resolved them against the project directory. Previously the raw path flowed into Path.GetFullPath, which on Windows trims trailing whitespace to an empty path and throws ArgumentException. Guard with string.IsNullOrWhiteSpace instead of string.IsNullOrEmpty so whitespace-only inputs keep their pre-migration behavior (throwing on Windows, resolving against the legacy base on Unix) instead of being silently re-rooted to the project directory. Adds a unit test demonstrating the mitigated behavior change. Addresses review feedback on dotnet#54621. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores pre-multithreading/back-compat behavior for whitespace-only ContentRoot/OutputPath inputs in StaticWebAsset.NormalizeContentRootPath(string, TaskEnvironment) by ensuring whitespace bypasses TaskEnvironment.GetAbsolutePath and flows directly into Path.GetFullPath.
Changes:
- Update
NormalizeContentRootPath(string, TaskEnvironment)to usestring.IsNullOrWhiteSpacewhen deciding whether to route throughTaskEnvironment.GetAbsolutePath. - Add a cross-platform regression test that verifies whitespace-only paths are not silently re-rooted to the project directory (throwing on Windows; resolving against process CWD on Unix).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/StaticWebAssetsSdk/Tasks/Data/StaticWebAsset.cs | Preserves legacy whitespace-path behavior by bypassing TaskEnvironment.GetAbsolutePath for null/empty/whitespace inputs. |
| test/Microsoft.NET.Sdk.StaticWebAssets.Tests/StaticWebAssets/StaticWebAssetTaskEnvironmentTests.cs | Adds a regression test validating whitespace-only path behavior remains consistent with the pre-migration semantics across OSes. |
Comment on lines
+1101
to
+1105
| // For null/empty/whitespace paths we deliberately call Path.GetFullPath on the raw value | ||
| // rather than routing it through env.GetAbsolutePath. This preserves the pre-multithreading | ||
| // behavior: such inputs resolve against the legacy base (and on Windows whitespace-only paths | ||
| // still throw, since Path.GetFullPath trims trailing whitespace to an empty path) instead of | ||
| // being silently resolved into the project directory. |
Replace the single runtime-branched test with two focused tests gated by [WindowsOnlyFact] and [LinuxOnlyFact], and shorten the comments. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #54621.
Veronika (review comment) raised a backwards-compatibility concern about
NormalizeContentRootPath(string path, TaskEnvironment env):The regression
Before the multi-threaded migration, the raw path flowed straight into
Path.GetFullPath. On Windows,Path.GetFullPath(" ")trims the trailing whitespace to an empty path and throwsArgumentException. After the migration the path was first routed throughTaskEnvironment.GetAbsolutePath, which resolves it against the project directory — so an invalid whitespaceOutputPath/ContentRootwas silently turned into the project directory instead of throwing.The fix (backwards compatible)
Guard with
string.IsNullOrWhiteSpaceinstead ofstring.IsNullOrEmpty:Whitespace-only inputs now bypass
env.GetAbsolutePathand flow straight intoPath.GetFullPath, exactly as before the migration:ArgumentException(invalid path), as it did pre-migration.Valid relative paths are unaffected and still resolve against the project directory (MT-safe).
Test proving the mitigated behavior change
NormalizeContentRootPath_WithWhitespacePath_PreservesLegacyBehavior_DoesNotResolveAgainstProjectDirectoryruns under a decoy CWD distinct from the project directory and asserts that a whitespace path is not re-rooted to the project directory (throws on Windows; resolves against the CWD on Unix). I verified the test fails against the previousIsNullOrEmptyimplementation and passes with this fix.Validation
./.dotnet/dotnet build test/Microsoft.NET.Sdk.StaticWebAssets.Tests/Microsoft.NET.Sdk.StaticWebAssets.Tests.csproj -c Debug*NormalizeContentRootPath*(4 tests) — all pass; new test fails on the pre-fix code.