Skip Authenticode for ReconnectModal.razor.js; add catalog signing - #35547
Skip Authenticode for ReconnectModal.razor.js; add catalog signing#35547jesuszarate wants to merge 1 commit into
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35547Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35547" |
|
Hey there @@jesuszarate! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
2de5fe1 to
4eea2ed
Compare
4eea2ed to
7d7b7f2
Compare
ReconnectModal.razor.js is a customer-modifiable template file in the maui-blazor-solution template. Customers edit it after project creation, so Authenticode signing is inappropriate. - Set CertificateName=None for ReconnectModal.razor.js (skip Authenticode) - Add eng/generate-catalog.ps1 to produce a .cat file covering *.js templates - Add GenerateCatalogFiles target to Microsoft.Maui.Templates.csproj - Add .cat FileExtensionSignInfo (Microsoft400) in eng/Signing.props
7d7b7f2 to
0b53911
Compare
|
/review -b feature/refactor-copilot-yml |
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
| Windows-only: makecat.exe ships with the Windows SDK. | ||
| --> | ||
| <Target Name="GenerateCatalogFiles" | ||
| AfterTargets="Build" |
There was a problem hiding this comment.
[major] Build & MSBuild — GenerateCatalogFiles is hooked to AfterTargets="Build" and the Exec always passes -ErrorIfMakecatNotFound, so every ordinary Windows build of the templates project now requires Windows SDK makecat.exe even when not packing/signing. Catalog generation is only needed for package/signing output; scope this target to the pack/signing path, e.g. remove the AfterTargets="Build" hook and keep/replace it with an appropriate pack-only target/property guard.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
| Windows-only: makecat.exe ships with the Windows SDK. | ||
| --> | ||
| <Target Name="GenerateCatalogFiles" | ||
| AfterTargets="Build" |
There was a problem hiding this comment.
[major] Build & MSBuild — GenerateCatalogFiles is wired with AfterTargets="Build"/BeforeTargets="Pack", which gives two bad failure modes: ordinary Windows builds now require makecat.exe because the target always passes -ErrorIfMakecatNotFound, and dotnet pack --no-build can run NuGet's pack dependency chain (BeforePack, GenerateNuspec, _GetPackageFiles) before this BeforeTargets="Pack" target adds the generated .cat to @(Content). Move catalog generation into the existing BeforePack dependency list (for example append GenerateCatalogFiles to <BeforePack>) and remove the AfterTargets/BeforeTargets hooks so the catalog is generated only for pack and before package content collection.
kubaflo
left a comment
There was a problem hiding this comment.
Could you check the ai's suggestions?
This comment has been minimized.
This comment has been minimized.
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 2 findings
See inline comments for details.
| --> | ||
| <Target Name="GenerateCatalogFiles" | ||
| AfterTargets="Build" | ||
| BeforeTargets="Pack" |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Build & MSBuild / signing packaging semantics — BeforeTargets="Pack" is too late to make the dynamically-added .cat content reliable for no-build packing. SDK Pack runs GenerateNuspec/_GetPackageFiles as dependencies before the Pack target body; when packing with NoBuild=true (or any flow where Build is not run in this invocation), this hook fires only after NuGet has already snapshotted @(Content), so maui-template-content.cat can be omitted from the .nupkg. Move catalog generation into BeforePack/GenerateNuspecDependsOn before _GetPackageFiles (or add the Content item statically with an Exists condition) so no-build pack still packages the signed catalog.
| Windows-only: makecat.exe ships with the Windows SDK. | ||
| --> | ||
| <Target Name="GenerateCatalogFiles" | ||
| AfterTargets="Build" |
There was a problem hiding this comment.
🔍 AI-Generated Review (multi-model)
[major] Build & MSBuild / safety — Hooking catalog generation to every Windows Build makes ordinary builds depend on makecat.exe: the Exec below always passes -ErrorIfMakecatNotFound, so a developer or CI leg that only builds this project on Windows without the Windows SDK catalog tool now fails even though it is not packing or signing. Restrict this target (and the hard-error behavior) to the pack/signing path instead of AfterTargets="Build".
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@jesuszarate — new AI review results are available based on this last commit:
0b53911. To request a fresh review after new comments or commits, comment/review rerun.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ⚠️ INCONCLUSIVE
Platform: ANDROID
⚠️ verify-tests-fail.ps1exited before writing a verification report. Diagnostics below.
Exit code: 3
Likely cause:
- Test detection failed — no runnable tests were found in the PR diff.
Gate output log (last 60 lines)
📁 Output directory: CustomAgentLogsTmp/PRState/35547/PRAgent/gate/verify-tests-fail
🔍 Detecting base branch and merge point...
No PR detected, scanning remote branches for closest base...
✅ Base branch: main (via closest-merge-base)
✅ Merge base commit: 6e107357
(1 commits ahead of main)
╔═══════════════════════════════════════════════════════════╗
║ FULL VERIFICATION MODE ║
╠═══════════════════════════════════════════════════════════╣
║ Fix files detected - will verify: ║
║ 1. Tests FAIL without fix ║
║ 2. Tests PASS with fix ║
╚═══════════════════════════════════════════════════════════╝
✅ Fix files (3):
- eng/Signing.props
- eng/generate-catalog.ps1
- src/Templates/src/Microsoft.Maui.Templates.csproj
🔍 Auto-detecting test filter from changed test files...
⚠️ No tests detected in this PR.
Searched for: UI tests, unit tests, XAML tests, device tests
Consider adding tests via write-tests-agent.
📋 Pre-Flight — Context & Validation
Issue: Unknown - GitHub CLI auth unavailable; no linked issue could be fetched.
PR: #35547 - Skip Authenticode for ReconnectModal.razor.js; add catalog signing
Platforms Affected: Templates/signing infrastructure; Android requested for candidate testing, but the changed code is pack/signing infrastructure and Windows-only catalog generation.
Files Changed: 3 implementation, 0 test
Key Findings
- PR changes
eng/Signing.props, addseng/generate-catalog.ps1, and updatessrc/Templates/src/Microsoft.Maui.Templates.csproj. - The PR's fix skips Authenticode for
ReconnectModal.razor.js, generates a catalog for template JavaScript files, includes that catalog in the templates package, and configures.catsigning. - GitHub context collection was partially blocked because
ghis unauthenticated; local branch diff and the patch endpoint were used instead. - The relevant template instructions were read: template project changes should preserve template packaging semantics and avoid generated files such as
cgmanifest.json. - Impacted UI test categories: NONE — this is not a UI runtime/control change.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ✗
src/Templates/src/Microsoft.Maui.Templates.csproj:132-140—GenerateCatalogFilesis wiredAfterTargets="Build"andBeforeTargets="Pack", causing ordinary Windows builds to requiremakecat.exewhile potentially running too late for pack content collection.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35547 | Skip direct signing for ReconnectModal.razor.js, generate maui-template-content.cat during build/pack, include it in package content, and sign .cat files. |
eng/Signing.props, eng/generate-catalog.ps1, src/Templates/src/Microsoft.Maui.Templates.csproj |
Original PR; gate was already inconclusive and was not re-run. |
🔬 Code Review — Deep Analysis
Code Review — PR #35547
Independent Assessment
What this changes: Exempts ReconnectModal.razor.js from Authenticode signing, generates a .cat catalog for template *.js files, packs it into Microsoft.Maui.Templates, and configures .cat signing.
Inferred motivation: The JS template file is customer-modifiable, so direct signing is inappropriate; catalog signing is meant to satisfy signing compliance.
Reconciliation with PR Narrative
Author claims: Matches the implementation intent.
Agreement/disagreement: The signing goal is sound, but the MSBuild target is wired incorrectly and leaves build/pack failure modes.
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
[major] GenerateCatalogFiles runs on ordinary Windows builds and requires makecat.exe. |
MauiBot inline reviews | ❌ Unresolved | Target still has AfterTargets="Build" at src/Templates/src/Microsoft.Maui.Templates.csproj:132 and always passes -ErrorIfMakecatNotFound at line 140. |
[major] BeforeTargets="Pack" can run too late for dotnet pack --no-build, after package content collection. |
MauiBot inline reviews / AI summary | ❌ Unresolved | Target still uses BeforeTargets="Pack" at line 133; NuGet pack targets collect content via GenerateNuspec / _GetPackageFiles before the Pack target body. |
Issue-comment surface could not be re-fetched because gh is not authenticated in this environment.
Blast Radius Assessment
- Runs for all instances: Yes, every Windows build of the Templates project.
- Startup impact: No app startup impact.
- Static/shared state: No.
- Infrastructure impact: Yes, templates packaging/signing path.
CI Status
- Required-check result: undetermined.
- Classification: tool-unavailable / undetermined.
- Action taken:
gh pr checks --requiredfailed due missing GitHub auth. Confidence capped low.
Findings
❌ Error — Catalog target runs in the wrong build/pack phase
src/Templates/src/Microsoft.Maui.Templates.csproj:132-140
GenerateCatalogFiles runs after every Windows Build and always fails if makecat.exe is unavailable, even though catalog generation is only needed for pack/signing output. Conversely, BeforeTargets="Pack" can be too late for dotnet pack --no-build, where package content may already be collected before this target adds the .cat.
Move catalog generation into the pack dependency path before _GetPackageFiles / GenerateNuspec content collection, e.g. via $(BeforePack), and avoid ordinary-build execution.
Failure-Mode Probing
- Windows build without Windows SDK: fails because
-ErrorIfMakecatNotFoundis always passed. dotnet pack --no-build: can omitmaui-template-content.catwhile JS is no longer directly signed.- Non-Windows build: target is skipped; no direct build break.
Verdict: NEEDS_CHANGES
Confidence: low
Summary: The catalog-signing approach is reasonable, but the target ordering/conditions are not safe. Prior major findings remain unresolved, and CI status could not be verified.
🛠️ Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | Move catalog generation into the existing BeforePack property dependency chain and remove build-time target hook. |
2 files | XML valid; build blocked by missing .NET 11 SDK. Likely viable and uses existing project convention. | |
| 2 | try-fix | Add catalog generation to GenerateNuspecDependsOn so it runs in NuGet package generation. |
2 files | XML valid; build blocked by missing .NET 11 SDK; expert self-review found ordering flaw because this can still run after _GetPackageFiles. |
|
| 3 | try-fix | Use TargetsForTfmSpecificContentInPackage and emit TfmSpecificPackageFile for the generated catalog. |
2 files | XML valid; build blocked by missing .NET 11 SDK. Likely viable, but more NuGet-pack-specific. | |
| 4 | try-fix | Hook GenerateCatalogFiles directly before NuGet _GetPackageFiles. |
2 files | XML valid; build blocked by missing .NET 11 SDK. Likely fixes timing, but depends on internal NuGet target name. | |
| 5 | try-fix | Predeclare the .cat as static pack content and generate the fixed path from BeforePack. |
2 files | XML valid; build blocked by missing .NET 11 SDK. Best unverified candidate: avoids dynamic item timing without internal target coupling. | |
| PR | PR #35547 | Generate catalog using AfterTargets="Build"/BeforeTargets="Pack". |
3 files | Original PR; code review found build and pack-ordering concerns. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| claude-opus-4.6 | 2 | Yes | Split/pack-only target before _GetPackageFiles; covered by candidate 4. |
| claude-opus-4.7 | 2 | Yes | Hook directly BeforeTargets="_GetPackageFiles"; covered by candidate 4. |
| gpt-5.3-codex | 2 | Yes | Pack-only dependency plus _GetPackageFiles timing; covered by candidate 4. |
| gpt-5.5 | 2 | Yes | Post-pack nupkg mutation/injection; rejected by viability review due stale copy/signing/ZIP mutation risks. |
| claude-opus-4.6 | 3 | No | Remaining variants are mechanical reshuffles of the same pre-pack hook points. |
| claude-opus-4.7 | 3 | Yes | Tokenized template JS; rejected as not applicable because the issue is catalog signing, not generated JS. |
| gpt-5.3-codex | 3 | No | Meaningful MSBuild extensibility points exhausted. |
| gpt-5.5 | 3 | Yes | Static .cat pack item plus BeforePack generation; covered by candidate 5. |
Exhausted: Yes
Selected Fix: No verified passing candidate. Candidate #5 is the strongest unverified alternative because it avoids the PR's ordinary-build execution and late dynamic item timing while avoiding internal NuGet target coupling. Candidate #1 is the simpler fallback if predeclaring a generated Content item proves incompatible with NuGet pack.
📝 Recommended PR Title & Description
Assessment: ✏️ Recommend updating — the current description still says GenerateCatalogFiles runs after Build/before Pack, but the winning fix moves it into BeforePack and predeclares the .cat pack item.
Recommended title
Templates: Skip Authenticode for ReconnectModal.razor.js; add catalog signing
Recommended description
ReconnectModal.razor.js is a customer-modifiable template file in the maui-blazor-solution template. Users edit it after project creation, so Authenticode signing is inappropriate -- it would break on modification.
This PR:
- Sets `CertificateName=None` for ReconnectModal.razor.js to skip Authenticode signing.
- Adds `eng/generate-catalog.ps1` to produce a `.cat` file covering `*.js` template content via `makecat.exe`.
- Adds a Windows-only `GenerateCatalogFiles` MSBuild target in `Microsoft.Maui.Templates.csproj` and runs it through the existing `BeforePack` chain so ordinary builds do not require `makecat.exe`.
- Predeclares `$(IntermediateOutputPath)maui-template-content.cat` as NuGet package content so pack content discovery sees the catalog before `_GetPackageFiles` snapshots package files.
- Adds `FileExtensionSignInfo` for `.cat` (Microsoft400) in `eng/Signing.props` so Arcade signs the catalog file.
The `.cat` provides integrity verification for the JS files without blocking customer modifications. This addresses VS signing compliance scan findings for the `mauitemplatesnet*` payloads (ReconnectModal.razor.js flagged as unsigned).
🏁 Report — Final Recommendation
Comparative Report — PR #35547
Summary
Winning candidate: pr-plus-reviewer
pr-plus-reviewer keeps the PR's intended signing design and applies the expert review fix for MSBuild/NuGet pack ordering: generate the catalog only in the pack path, and predeclare the .cat package item before NuGet snapshots content. No candidate has a passing regression result because local verification was blocked by environment/tooling, but no candidate is recorded as failing regression tests.
Candidate ranking
| Rank | Candidate | Regression result | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
Inconclusive / XML-valid sandbox | Best choice. It preserves the PR's signing solution and fixes both expert findings: no ordinary-build makecat.exe dependency and reliable pack content discovery. |
| 2 | try-fix-5 |
Blocked / XML OK | Same core approach as the reviewer-applied candidate: static .cat pack item plus BeforePack materialization. Strongest standalone try-fix candidate, but pr-plus-reviewer is preferred because it is the PR fix with review feedback applied. |
| 3 | try-fix-3 |
Blocked / XML OK | Uses TargetsForTfmSpecificContentInPackage and TfmSpecificPackageFile, which directly targets pack content discovery. Viable but more NuGet-pack-specific than using this project's existing BeforePack convention. |
| 4 | try-fix-1 |
Blocked / XML OK | Moves generation into BeforePack, avoiding ordinary build execution. Less robust than try-fix-5 because it still dynamically adds Content inside the target instead of predeclaring the pack item. |
| 5 | try-fix-4 |
Blocked / XML OK | Hooks directly before NuGet's internal _GetPackageFiles target. It addresses timing but depends on an internal target name, making it more brittle than BeforePack plus static content. |
| 6 | pr |
Gate inconclusive | Implements the desired signing/catalog strategy but has two major MSBuild issues: build-time makecat.exe coupling and unreliable no-build pack content timing. |
| 7 | try-fix-2 |
Blocked / self-review failed | Avoids ordinary build execution, but its GenerateNuspecDependsOn hook is likely too late because NuGet prepends _GetPackageFiles before the appended dependency value. |
Why pr-plus-reviewer wins
The raw PR has the right high-level approach but wires catalog generation around Build/Pack in a way that can break ordinary Windows builds and miss the .cat in no-build pack flows. pr-plus-reviewer corrects that by using the templates project's existing BeforePack chain and by declaring the .cat pack item at evaluation time. This is the least brittle fix among the viable candidates and avoids the internal NuGet target coupling in try-fix-4.
Notes on test status
The gate was inconclusive because no runnable tests were detected for this signing/template packaging change. Try-fix validation was also blocked by the local SDK mismatch: the repository targets .NET 11.0 while the available SDK is 10.0.100. These blocked/inconclusive results are not regression failures, so they do not force a lower ranking under the failed-regression rule.
🧭 Next Steps — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
kubaflo
left a comment
There was a problem hiding this comment.
Could you please check the ai's suggestions?
kubaflo
left a comment
There was a problem hiding this comment.
Could you please resolve conflicts?
|
Closing as a stale one - please reopen or create a new one if this is still needed |
ReconnectModal.razor.js is a customer-modifiable template file in the maui-blazor-solution template. Users edit it after project creation, so Authenticode signing is inappropriate -- it would break on modification.
This PR:
CertificateName=Nonefor ReconnectModal.razor.js to skip Authenticode signing.eng/generate-catalog.ps1to produce a.catfile covering*.jstemplate content viamakecat.exe.GenerateCatalogFilesMSBuild target inMicrosoft.Maui.Templates.csprojthat runs after Build/before Pack (Windows-only). The.catis included in the NuGet package.FileExtensionSignInfofor.cat(Microsoft400) ineng/Signing.propsso Arcade signs the catalog file.The
.catprovides integrity verification for the JS files without blocking customer modifications. This addresses VS signing compliance scan findings for themauitemplatesnet*payloads (ReconnectModal.razor.js flagged as unsigned).