You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Reject malformed publisher DNs in cert generate (#845) - #892
winapp cert generate --publisher (and manifest generate --publisher-name, which share the same normalization) accepted malformed X.500 distinguished names (DNs) and silently generated a certificate or manifest that can never match the MSIX Identity/@Publisher — exit 0, a .pfx on disk, and a failure that only surfaces much later as an unexplained signing/install error. Verified against issue #845.
PublisherDnHelper now validates the publisher up front and rejects, with a message naming the exact problem, any value the MSIX manifest publisher cannot faithfully represent:
--publisher input
Old behavior
New behavior
CN=
CN="" (empty CN)
exit 1 — names the empty CN
CN=A, O=
empty O
exit 1 — names the empty O
=Contoso / CN=A,,O=B
wrapped as a literal CN
exit 1 — not a valid DN
OID.2.5.4.3=A,,O=B
wrapped as a literal CN
exit 1 — not a valid DN
CN=A+O=B (multi-valued RDN)
empty value could hide
exit 1 — multi-valued RDN unsupported
CN=A;O=B (semicolon separator)
cert used commas, manifest kept ;
exit 1 — use comma separators
CN=A, / CN=A+ (trailing separator)
cert dropped it, manifest kept it
exit 1 — remove the trailing separator
CN=Contoso\Bar (backslash)
unmatchable in the manifest
exit 1 — backslash unsupported
"" (explicit empty)
silently used the default publisher
exit 1 — publisher cannot be empty
Valid input is unaffected and round-trips as before: CN=Contoso Inc, O=Contoso, C=US, bare names like Contoso (wrapped as CN=Contoso), bare names that merely contain = (e.g. R&D = Team), and quoted data such as CN="A,". A new non-throwing TryNormalize lets the command pre-validate an explicit --publisher and fail cleanly — emitting a JSON error envelope under --json — instead of falling through to a default. This complements #890's --manifest extraction (merged from main): --manifest still supplies the publisher when no explicit --publisher is given, and an explicit --publisher is validated even alongside --manifest.
Usage Example
$ winapp cert generate --publisher "CN=A,,O=B"❌ Publisher 'CN=A,,O=B' is not a valid distinguished name (DN). Use a plain name (wrapped as CN=<name>) or a valid DN such as 'CN=Contoso, O=Contoso, C=US'.
$ echo$LASTEXITCODE1
$ winapp cert generate --publisher "CN=Contoso, O=Contoso, C=US"✅ Development certificate generated successfully.
Shipped skills updated in plugins/winapp/skills/ (if CLI commands/workflows changed)
Additional Notes
Documentation was aligned to the narrowed contract on every surface that promised "any valid X.500 DN": docs/usage.md, the cert generate / manifest generate option descriptions (and the regenerated docs/cli-schema.json), plugins/winapp/com.github.copilot/agents/winapp.agent.md, and plugins/winapp/skills/winapp-signing/SKILL.md.
Local validation used the Debug CLI: the NativeAOT-published binary can't be built in this environment (missing VS native linker), and these are argument-parsing/validation-logic changes that aren't AOT-sensitive. The pipeline's build-and-package (including NativeAOT publish, full test shards, docs validation, and sample builds) is green on the pre-merge head; CI re-runs on the merge commit.
`winapp cert generate --publisher` accepted malformed X.500 distinguished
names and silently produced a certificate that could never match the
manifest Identity/@publisher, surfacing later as an unexplained install
failure.
PublisherDnHelper.Normalize now rejects, with a message naming the problem:
- DNs with an empty-valued component (e.g. `CN=`, `CN=A, O=`)
- multi-valued RDNs (e.g. `CN=A+O=`), which are unused for publishers and
can hide an empty value
- input that begins like a DN attribute assignment but does not parse
(e.g. `=Contoso`, `CN=A,,O=B`)
A plain name that merely contains `=` further in (e.g. `R&D = Team`) is
still wrapped as `CN=<name>`. A new non-throwing TryNormalize is used by the
command to pre-validate an explicit --publisher (including an explicitly
empty value) and fail with exit 1 and a JSON error envelope under --json,
instead of silently falling back to the default publisher.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…DN limits
- Reject any publisher containing a backslash (literal or X.500 escape such as
`CN=Contoso\Bar` / `CN=Contoso\, Inc`) before both the parsed-DN and bare-name
paths — the MSIX manifest publisher type has no escape sequences, so such a
certificate can never match Identity/@publisher (the remaining case from #845).
- Recognize the standard `OID.`-prefixed attribute form (e.g.
`OID.2.5.4.3=A,,O=B`) as a distinguished-name attempt so a malformed one is
rejected instead of silently wrapped as a literal CN.
- Document the single-valued, comma-separated RDN requirement (no multi-valued
`+` RDNs, no backslashes) on docs/usage.md and the shipped guidance that
previously promised "any valid X.500 DN" (winapp.agent.md, winapp-signing SKILL).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Semicolon-separated DNs still produce a mismatched publisher.X500DistinguishedName accepts CN=A;O=B, so this branch returns the raw semicolon form. Reproducing with the built CLI, winapp manifest generate --publisher-name 'CN=A;O=B' writes Publisher="CN=A;O=B", while winapp cert generate exits 0 and cert info reports CN=A, O=B. MSIX requires comma-separated components, so the generated manifest and certificate cannot match. Reject unquoted semicolon separators (or canonicalize consistently before writing either artifact) and add this input as a regression case.
Other suites are reflected in the overall validation status above.
✅ 7667 passed, 37 skipped out of 7704 tests in 1196.4s (+44 tests, -83.5s vs. baseline)
Test Coverage
✅ 85.7% line coverage, 79.9% branch coverage · ✅ +0.1% vs. baseline
CLI Startup Time
65ms median (x64, winapp --version) · ✅ +8ms vs. baseline
Try This Build
Installs the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing.
- Reject an unquoted ';' RDN separator (e.g. `CN=A;O=B`). .NET parses ';' as an
X.500 separator equivalent to ',', but the raw string keeps the literal ';':
the certificate re-parses it to comma-separated RDNs while the manifest writes
the literal ';', so the two artifacts silently mismatch. A ';' inside a quoted
value (`CN="A;B"`) is data and is still accepted.
- Update the source option descriptions for `cert generate --publisher` and
`manifest generate --publisher-name` (previously "any valid X.500 DN") and
regenerate docs/cli-schema.json so --help, the schema, and docs state one
consistent contract: single-valued, comma-separated components only.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The prior schema regen ran against a local Debug build, whose CLI reports the
default assembly version 1.0.0 (the real version is injected only by the build
script). That overwrote the correct 0.6.3, so validate-docs — which normalizes
the CLI's version to version.json before comparing — flagged drift. The option
description edits from that regen are correct and are kept.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
A trailing ',' or '+' (e.g. CN=A,) parses as a valid DN, but
X500DistinguishedName drops the empty trailing RDN when it encodes the
certificate, so the subject becomes CN=A while the manifest keeps the literal
CN=A,. EnumerateRelativeDistinguishedNames() also omits that empty element, so
the component check could not see it. Reject a trailing unquoted separator from
the raw string before enumeration; a separator inside a quoted value (CN="A,")
is data and is still accepted. Adds regression tests for both forms.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
State: ready-for-review (technical gates verified; not approval or merge permission) Assessed: head 3bab9313aea7f0fb01eaf2e38e51709ab7d85728 against main@bb02c145 Evidence: required CI green on this head — build-and-package SUCCESS (NativeAOT publish, CLI test shards, validate-docs, UI e2e, all sample builds); 38 checks, 0 failures. Locally: build clean, cert/helper suites green, validate-llm-docs in sync, every rejected/accepted publisher form smoke-tested on the CLI. Review: every Copilot inline finding across all rounds addressed and resolved (empty/malformed DN, backslash, OID. prefix, multi-valued RDN, semicolon separator, trailing separator, help/schema alignment). 0 unresolved threads. Human approval still required (reviewDecision: REVIEW_REQUIRED). Base: main integrated (#882/#890/#891/#907, then #884). Main has since advanced one unrelated commit (#878, no file overlap, clean merge); under this repo's strict up-to-date rule the branch needs a routine Update branch immediately before merge. This is a merge-time formality, not a technical defect. Pending: human review/approval; final branch update at merge time. Blocker / owner / next step: none blocking; awaiting a human reviewer. Resume: Not actively monitoring — will reassess on request or if a new commit/finding lands.
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
ready-for-reviewAgent work and technical checks complete; awaiting review or re-review, not approval or merge
3 participants
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.
Description
winapp cert generate --publisher(andmanifest generate --publisher-name, which share the same normalization) accepted malformed X.500 distinguished names (DNs) and silently generated a certificate or manifest that can never match the MSIXIdentity/@Publisher— exit 0, a.pfxon disk, and a failure that only surfaces much later as an unexplained signing/install error. Verified against issue #845.PublisherDnHelpernow validates the publisher up front and rejects, with a message naming the exact problem, any value the MSIX manifest publisher cannot faithfully represent:--publisherinputCN=CN=""(empty CN)CNCN=A, O=OO=Contoso/CN=A,,O=BOID.2.5.4.3=A,,O=BCN=A+O=B(multi-valued RDN)CN=A;O=B(semicolon separator);CN=A,/CN=A+(trailing separator)CN=Contoso\Bar(backslash)""(explicit empty)Valid input is unaffected and round-trips as before:
CN=Contoso Inc, O=Contoso, C=US, bare names likeContoso(wrapped asCN=Contoso), bare names that merely contain=(e.g.R&D = Team), and quoted data such asCN="A,". A new non-throwingTryNormalizelets the command pre-validate an explicit--publisherand fail cleanly — emitting a JSON error envelope under--json— instead of falling through to a default. This complements #890's--manifestextraction (merged from main):--manifeststill supplies the publisher when no explicit--publisheris given, and an explicit--publisheris validated even alongside--manifest.Usage Example
Related Issue
Fixes #845.
Type of Change
Checklist
plugins/winapp/skills/(if CLI commands/workflows changed)Additional Notes
Documentation was aligned to the narrowed contract on every surface that promised "any valid X.500 DN":
docs/usage.md, thecert generate/manifest generateoption descriptions (and the regenerateddocs/cli-schema.json),plugins/winapp/com.github.copilot/agents/winapp.agent.md, andplugins/winapp/skills/winapp-signing/SKILL.md.Local validation used the Debug CLI: the NativeAOT-published binary can't be built in this environment (missing VS native linker), and these are argument-parsing/validation-logic changes that aren't AOT-sensitive. The pipeline's
build-and-package(including NativeAOT publish, full test shards, docs validation, and sample builds) is green on the pre-merge head; CI re-runs on the merge commit.