Fix SRI integrity failures in Blazor WASM incremental builds#52847
Fix SRI integrity failures in Blazor WASM incremental builds#52847lewing wants to merge 1 commit intodotnet:mainfrom
Conversation
During incremental builds, the compression task could read stale content from the destination file (wwwroot\_framework\dotnet.js) instead of the updated source file (obj\...\dotnet.js) because the file copy hadn't happened yet. This caused SRI integrity check failures when: 1. _GenerateBuildWasmBootJson creates obj\dotnet.js with NEW fingerprints 2. GenerateBuildCompressedStaticWebAssets compresses wwwroot\dotnet.js (OLD) 3. _BuildStaticWebAssetsPreserveNewest copies to wwwroot\dotnet.js (too late) Fix: When both RelatedAsset and RelatedAssetOriginalItemSpec exist and point to different files, compare timestamps and prefer the newer file. This ensures the compression task uses the freshly-generated source file during incremental builds, while still preferring RelatedAsset in the normal case after copying. Fixes: dotnet/aspnetcore#65271
github-actions
bot
added
the
Area-AspNetCore
|
Thanks for your PR, @@lewing. |
There was a problem hiding this comment.
Pull request overview
This PR fixes SRI (Subresource Integrity) integrity failures in Blazor WebAssembly incremental builds by ensuring the compression task uses the most recently updated asset file. The issue occurred when the compression task read stale content from the destination file because the file copy operation hadn't completed yet during incremental builds.
Changes:
- Added timestamp comparison logic to
AssetToCompress.TryFindInputFilePaththat prefers the newer file when bothRelatedAssetandRelatedAssetOriginalItemSpecexist and point to different files - Added comprehensive unit tests covering the bug scenario, normal post-copy scenario, same-file scenario, and existing edge cases
- Updated existing tests to explicitly set timestamps to ensure deterministic behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/StaticWebAssetsSdk/Tasks/Utils/AssetToCompress.cs | Added timestamp comparison logic to prefer the newer file when both source and destination exist, fixing the incremental build race condition |
| test/Microsoft.NET.Sdk.StaticWebAssets.Tests/StaticWebAssets/AssetToCompressTest.cs | Added three new tests for timestamp-based file selection and updated two existing tests to explicitly set file timestamps for deterministic behavior |
| var relatedAsset = assetToCompress.GetMetadata("RelatedAsset"); | ||
| var relatedAssetOriginalItemSpec = assetToCompress.GetMetadata("RelatedAssetOriginalItemSpec"); | ||
|
|
||
| var relatedAssetExists = File.Exists(relatedAsset); | ||
| var originalItemSpecExists = File.Exists(relatedAssetOriginalItemSpec); | ||
|
|
||
| // When both paths exist and point to different files, prefer the newer one. | ||
| // This handles incremental builds where the source file (OriginalItemSpec) may be | ||
| // newer than the destination (RelatedAsset), which hasn't been copied yet. | ||
| if (relatedAssetExists && originalItemSpecExists && | ||
| !string.Equals(relatedAsset, relatedAssetOriginalItemSpec, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| var relatedAssetTime = File.GetLastWriteTimeUtc(relatedAsset); | ||
| var originalItemSpecTime = File.GetLastWriteTimeUtc(relatedAssetOriginalItemSpec); | ||
|
|
||
| if (originalItemSpecTime > relatedAssetTime) | ||
| { | ||
| log.LogMessage(MessageImportance.Low, "Asset '{0}' using original item spec '{1}' because it is newer than '{2}'.", | ||
| assetToCompress.ItemSpec, | ||
| relatedAssetOriginalItemSpec, | ||
| relatedAsset); | ||
| fullPath = relatedAssetOriginalItemSpec; | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Ugh, no. This task shouldn't worry about incrementalism.
This is an artifact of the magic that we do on webassembly where we define the asset pointing to a non-existing place. We should do that as opposed to checking for newer files here.
this is ~4 file accesses per compression check that will get very costly soon. DefineStaticWebAssets is the only task that is "allowed" to read the file from disk to ensure we only do it once per file. Other than when we need to actually process the file.
This now points out to a bigger problem (surprised that we haven't seen it before) where the outputs from the build are considered inputs to the current one.
We need to do this on the wasm sdk, otherwise doing it here will destroy perf. One way to do this would be to delete the file at the beginning of the build when it changes, but better yet would be to stop defining these assets with an item spec in the wwwroot folder and just define them in their original location on disk.
There was a problem hiding this comment.
yeah, I was certain this would annoy you, I think in the common cases it only adds ~one more file exists call (both calls were already in the code the second was just short circuited) but I confess I'm not that familiar with exactly what amounts to common for these cases. The regression is unfortunate so whatever we do lets do it quickly.
|
closed in favor of dotnet/runtime#124125 |
2 participants
Summary
Fixes dotnet/aspnetcore#65271
During incremental builds, the compression task could read stale content from the destination file (\wwwroot_framework\dotnet.js) instead of the updated source file (\obj...\dotnet.js) because the file copy hadn't happened yet.
This caused SRI integrity check failures when:
Regression
This was caused by #52283 which changed the order to prefer \RelatedAsset\ over \RelatedAssetOriginalItemSpec. While that fixed the esproj scenario, it broke incremental builds where the source file is newer than the stale destination.
Fix
When both \RelatedAsset\ and \RelatedAssetOriginalItemSpec\ exist and point to different files, compare timestamps and prefer the newer file. This ensures the compression task uses the freshly-generated source file during incremental builds, while still preferring \RelatedAsset\ in the normal case after copying.
Testing