Fix cert bug with multi-component publisher validation

[zateutsch]

fixes #567

Users with multi-component publisher DNs got a false mismatch error during winapp package. The certificate was generated correctly, but ExtractPublisherFromCertificate only extracted the CN value via regex CN=([^,]+), dropping L/S/C/O fields.

Fix

• Publisher normalization: Replaced .Replace("CN=", "") with proper DN detection — if input already starts with CN=, use it as-is; otherwise wrap with CN=.
• Publisher extraction: Return full cert.Subject instead of regex-extracting only the CN component.
• Publisher validation: Use X500DistinguishedName.RawData byte comparison for semantically correct DN matching.
• JSON output: Preserved backward compat — simple CN-only publishers still display as bare names in --json output.

Tests

Added 15 parameterized tests covering simple CN, GUID-style, locale fields, full corporate DN, org-only, country-only, spaces in values, bare name normalization, and whitespace trimming. Fixed test cert store cleanup to include all new subjects.

[Copilot]

Pull request overview

Fixes publisher mismatch errors during winapp package when the certificate subject uses a multi-component X.500 distinguished name (DN), by extracting and validating the full DN rather than only the CN component.

Changes:

• Normalizes publisher input so full DNs (CN=...,...) are preserved while bare names are wrapped as CN=....
• Extracts the full certificate subject DN and validates manifest vs certificate publisher via X500DistinguishedName.RawData comparison.
• Expands test coverage for multiple DN formats and updates test cert store cleanup subjects.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
src/winapp-CLI/WinApp.Cli/Services/CertificateService.cs	Normalizes publisher DN handling; returns full cert subject; compares publishers using X.500 raw bytes.
src/winapp-CLI/WinApp.Cli.Tests/PackageCommandTests.cs	Adds/updates tests and cleanup subjects for multi-component publisher DNs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[github-actions]

Build Metrics Report

Binary Sizes

Artifact	Baseline	Current	Delta
CLI (ARM64)	31.49 MB	31.49 MB	📈 +2.0 KB (+0.01%)
CLI (x64)	31.82 MB	31.83 MB	📈 +2.5 KB (+0.01%)
MSIX (ARM64)	13.23 MB	13.23 MB	📈 +0.2 KB (+0.00%)
MSIX (x64)	14.05 MB	14.05 MB	📉 -1.8 KB (-0.01%)
NPM Package	27.52 MB	27.53 MB	📈 +2.0 KB (+0.01%)
NuGet Package	27.62 MB	27.62 MB	📈 +2.6 KB (+0.01%)
VS Code Extension	20.42 MB	20.42 MB	📈 +2.5 KB (+0.01%)

Test Results

✅ 1265 passed, 1 skipped out of 1266 tests in 441.9s (+67 tests, -16.4s vs. baseline)

Test Coverage

❌ 17.3% line coverage, 36.5% branch coverage · ✅ no change vs. baseline

CLI Startup Time

33ms median (x64, winapp --version) · 📉 -13ms vs. baseline

---

Updated 2026-06-19 23:24:50 UTC · commit ab433d3 · workflow run

[nmetulev]

🤖 AI-generated review — produced by the pr-review skill (8 parallel review dimensions + a cross-model verification pass on GPT-5.4, orchestrated by Claude Opus 4.8). This is automated analysis, not a human review: findings may contain false positives, and nothing here has been built or tested. Treat it as a checklist of things to verify, not a merge gate.

PR Review — zt/567-cert-CN-validation-bug vs origin/main (11 commits, 21 files, +483 / −76)

Summary

Critical: 0 High: 0 Medium: 5 Low: 2

Solid, well-tested bug fix. The cross-model pass made two important corrections: (1) the DN parser is not hand-rolled — it delegates to the BCL X500DistinguishedName, so quoted values, lowercase attributes, and + multi-valued RDNs already work; and (2) the remaining DN bug causes false rejection only — no cert/publisher trust-bypass (the validation gate reparses and compares RawData). That dropped the lone High to Medium. The substantive items are: one residual DN edge-case bug (escaped commas), an unescaped PublisherDisplayName write, and a few test/UX/doc gaps.

Coverage

Dimension	Result
security	⚠ 1 finding (folded into M4)
correctness	⚠ 1 finding
cli-ux	⚠ 2 findings
alternative-solution	⚠ 2 findings (folded into M1/M4)
test-coverage	⚠ 2 findings
docs-and-samples	✓ clean
packaging	⚠ 2 findings (folded into M4/L2)
multi-model	0/1 highs confirmed · 1 downgraded to medium · 0 new

Findings

ID	File:lines	Domain	Finding
M1	src/winapp-CLI/WinApp.Cli/Helpers/PublisherDnHelper.cs:41-69	correctness, alternative-solution	Escaped-comma DNs (CN=Last\, First) rejected by the BCL ctor → double-prefixed to CN=CN=… (was High; cross-model downgraded)
M2	src/winapp-CLI/WinApp.Cli/Helpers/PublisherDnHelper.cs:77-128	test-coverage	DN edge cases untested: escaped comma, + multi-valued RDN, lowercase attrs, RDN ordering, null input
M3	src/winapp-CLI/WinApp.Cli/Services/CertificateService.cs:449-450	cli-ux	Mismatch "fix" hint uses manifestPath.Name only — fails when run outside the manifest folder
M4	src/winapp-CLI/WinApp.Cli/Services/ManifestTemplateService.cs:118-124	security, packaging, alternative-solution	{PublisherName} injected into the manifest without XML escaping (while {PublisherDN} is escaped)
M5	src/winapp-CLI/WinApp.Cli/Services/ManifestTemplateService.cs:233-260	test-coverage	No end-to-end test that a generated manifest's Publisher matches a generated cert's subject for complex DNs
L1	docs/usage.md:644-645	cli-ux	--publisher still documented as a plain "name"; doesn't mention DN support / bare-name wrapping
L2	src/winapp-npm/src/winapp-commands.ts:88-89	packaging	Generated npm wrapper doc-comment stale vs the updated CLI schema description

---

Details

M1 — src/winapp-CLI/WinApp.Cli/Helpers/PublisherDnHelper.cs:41-69 · correctness + alternative-solution · Multi-model: downgraded from High

• Severity: medium · Confidence: high
• Finding: RFC-style backslash-escaped value delimiters in a DN (e.g. CN=Last\, First) are misclassified and get an extra CN= prefix, producing CN=CN=Last\, First.
• Evidence: IsDistinguishedName delegates to new X500DistinguishedName(input) (lines 24-27) — so quoted values, lowercase attribute types, and + multi-valued RDNs already parse correctly. But that constructor rejects backslash-escaped delimiters, so Normalize falls through to return $"CN={trimmed}" (lines 64-69). Downstream, GenerateDevCertificateAsync passes the malformed subject into new CertificateRequest(...) (CertificateService.cs:42-63), which throws; the validation gate reparses both sides and compares RawData (CertificateService.cs:443-446). Net effect: false rejection / cert-generation failure — not a trust bypass (cross-model confirmed no false-match path).
• Recommendation: For the bare-name path, build the DN with X500DistinguishedNameBuilder.AddCommonName(trimmed) (available on net10.0-windows) instead of string interpolation, so the CN value is escaped correctly and escaped-comma inputs round-trip. Add the escaped-comma case to tests (see M2).

M4 — src/winapp-CLI/WinApp.Cli/Services/ManifestTemplateService.cs:118-124 · security + packaging + alternative-solution

• Severity: medium · Confidence: high
• Finding: The PublisherDisplayName value is written into the manifest via raw string .Replace without XML escaping, while the sibling {PublisherDN} replacement is escaped — so a publisher containing &, <, >, or " produces an invalid (or injected) manifest.
• Evidence: .Replace("{PublisherDN}", PublisherDnHelper.XmlEscape(publisherDN)) (121) vs .Replace("{PublisherName}", publisherName) (122); template uses <PublisherDisplayName>{PublisherName}</PublisherDisplayName>. AppxManifestDocument.cs:179-182 already exposes a typed IdentityPublisher.
• Recommendation: Preferred fix addresses all three domains at once: parse the template with AppxManifestDocument, set IdentityPublisher / IdentityName / IdentityVersion via typed properties, and serialize with ToXml() so XDocument handles escaping — instead of chained .Replace + manual XmlEscape. At minimum, also XmlEscape(publisherName). Add a manifest-generation test with CN=A&B.

M2 — src/winapp-CLI/WinApp.Cli/Helpers/PublisherDnHelper.cs:77-128 · test-coverage

• Severity: medium · Confidence: high
• Finding: DN display/component parsing lacks coverage for several edge cases the fix is meant to handle.
• Evidence: Tests cover simple CN, multi-RDN, non-CN, quoted comma, and empty/whitespace, but not escaped comma \,, + multi-valued RDNs, lowercase attribute types, RDN-ordering differences, separator-whitespace equivalence, or null input (PublisherDnHelperTests.cs:14-24, 33-40, 72-78).
• Recommendation: Add [DataRow] cases for those DN forms across IsDistinguishedName, Normalize, and GetDisplayName — the escaped-comma row would directly catch M1.

M5 — src/winapp-CLI/WinApp.Cli/Services/ManifestTemplateService.cs:233-260 · test-coverage

• Severity: medium · Confidence: high
• Finding: No end-to-end test verifies that a generated manifest's Publisher matches a generated certificate's subject for complex DNs — the exact scenario this PR fixes.
• Evidence: Certificate tests manually write/replace the manifest publisher before validation (PackageCommandTests.cs:755-760, 809-817); manifest-generation tests assert only simple CN=TestPublisher (ManifestCommandTests.cs:69-103).
• Recommendation: Add an integration test that generates a manifest with CN+O+C and a non-CN publisher, generates a cert from the same publisher, then asserts the manifest Identity/Publisher equals the cert SubjectName (RawData) — a true regression test for the reported bug.

M3 — src/winapp-CLI/WinApp.Cli/Services/CertificateService.cs:449-450 · cli-ux

• Severity: medium · Confidence: high
• Finding: The publisher-mismatch error suggests a fix command that drops the manifest's directory, so the suggested command can fail when the user isn't in the manifest folder.
• Evidence: Regenerate the certificate with 'winapp cert generate --manifest "{manifestPath.Name}"' (line 450) uses .Name (filename only).
• Recommendation: Use the resolved/cwd-relative manifest path in the suggested --manifest value so the copy-pasted command works as-is.

L1 — docs/usage.md:644-645 · cli-ux

• Severity: low · Confidence: high
• Finding: Usage docs still describe --publisher as a generic name, omitting the new DN support and bare-name wrapping behavior.
• Evidence: - `--publisher <name>` - Publisher name for certificate (line 645).
• Recommendation: Match the updated cert generate schema wording (DN support); fix other stale --publisher entries too.

L2 — src/winapp-npm/src/winapp-commands.ts:88-89 · packaging

• Severity: low · Confidence: high
• Finding: The autogenerated npm wrapper doc-comment wasn't regenerated after the CLI schema description changed.
• Evidence: /** Publisher name for the generated certificate. If not specified, will be inferred from manifest. */ vs the schema's new "Publisher distinguished name (DN)…".
• Recommendation: Run npm run generate-commands and include the regenerated winapp-commands.ts.

---

Clean dimension (what was verified)

• docs-and-samples: the hand-written docs/fragments/skills/winapp-cli/signing.md and its autogenerated mirrors (.github/plugin + .claude SKILL.md, winapp.agent.md) are consistent; docs/cli-schema.json was regenerated and matches the command descriptions; no stale guidance describing the old "must start with CN=" behavior remains.

---

_{Generated by the winappcli pr-review skill · 8 specialist passes + GPT-5.4 cross-model verification · 2026-06-19. No code was built or executed; verify findings before acting. 🤖}

[nmetulev]

Approving, contingent on reviewing and addressing any relevant feedback from the ai generated review