Repository navigation
Fix cert generate --manifest silently falling back to user name (#839) - #890
Merged
Nikola Metulev (nmetulev) merged 6 commits intoSep 21, 2026
Merged
Conversation
`winapp cert generate --manifest <path>` silently substituted the OS user
name as the certificate publisher when it could not extract
`Identity/@Publisher`, exiting 0 and reporting success. The resulting
certificate can never match the manifest, so the signed package fails to
install. This commonly happened with partially-complete manifests (valid
publisher, but no `<Applications>` element), because manifest parsing
required Name + Publisher + ApplicationId together.
- Add `MsixService.ExtractPublisherFromPathAsync`, which reads only
`Identity/@Publisher` (rejecting a missing/empty value) and no longer
requires an Application element or package Name.
- In `CertGenerateCommand`, when `--manifest` is named explicitly and no
`--publisher` overrides it, resolve the publisher up front and fail with a
clear error (non-zero exit; single `{"error":...}` under `--json`) instead
of falling back to the default. Auto-discovered project manifests stay
lenient.
- Document the behavior in docs/usage.md and add unit + command tests.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new extraction catch misreports command cancellation as invalid manifest input.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Fixes certificate publisher inference for incomplete manifests and fails clearly when explicit manifest extraction fails.
Changes:
- Adds publisher-only manifest extraction.
- Enforces text and JSON error behavior.
- Adds regression tests and documentation.
| File | Description |
|---|---|
MsixService.cs |
Adds publisher-only extraction. |
CertGenerateCommand.cs |
Validates explicit manifests before generation. |
MsixServicePackagingTests.cs |
Tests extraction edge cases. |
CertGenerateCommandTests.cs |
Tests command success and failure paths. |
docs/usage.md |
Documents explicit-manifest behavior. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Exclude OperationCanceledException from the manifest-publisher extraction catch in CertGenerateCommand so a cancellation mid-read is not misreported as a "Could not extract the publisher" input error (matches the repo's established `when (ex is not OperationCanceledException)` pattern). - Add a regression test asserting a canceled cert generate --manifest does not report the manifest error and creates no PFX. - Switch the new tests' path construction from Path.Combine to Path.Join. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
Build Metrics ReportBinary Sizes
Test Results✅ 7606 passed, 37 skipped out of 7643 tests in 1182.2s (+8 tests, -10.1s vs. baseline) Test Coverage✅ 85.7% line coverage, 79.9% branch coverage · ✅ no change vs. baseline CLI Startup Time65ms median (x64, Try This BuildInstalls 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. & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 890Switching between builds often?Put the tool on your PATH once: & ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPathThen this build is just: winapp-pr 890Run Updated 2026-09-19 22:30:38 UTC · commit |
Nikola Metulev (nmetulev)
approved these changes
Sep 21, 2026
Nikola Metulev (nmetulev)
enabled auto-merge (squash)
September 21, 2026 20:03
Nikola Metulev (nmetulev)
disabled auto-merge
September 21, 2026 20:47
Nikola Metulev (nmetulev)
deleted the
zateutsch-cert-manifest-publisher-fallback
branch
September 21, 2026 20:47
3 tasks done
4 tasks done
Nikola Metulev (nmetulev)
added a commit
that referenced
this pull request
Sep 21, 2026
## Description `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 ```console $ 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 $LASTEXITCODE 1 $ winapp cert generate --publisher "CN=Contoso, O=Contoso, C=US" ✅ Development certificate generated successfully. ``` ## Related Issue Fixes #845. ## Type of Change - 🐛 Bug fix ## Checklist - [x] New tests added for new functionality (if applicable) - [x] Tested locally on Windows - [x] [docs/usage.md](../docs/usage.md) updated (if CLI commands changed) - [x] 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. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Nikola Metulev <nmetulev@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.
Summary
Fixes #839.
winapp cert generate --manifest <path>silently substituted the OS user name as the certificate publisher when it could not extractIdentity/@Publisherfrom the manifest — exiting0and reporting success. Because the publisher can never matchIdentity/@Publisher, the resulting certificate produces a package that fails to install, while the CLI reports success.This happened for partially-complete manifests (valid
Identity/@Publisher, but missing<Applications>), which are common mid-development — exactly when a dev certificate gets generated. The cause: manifest parsing (ParseAppxManifestFromPathAsync) requiredName+Publisher+ApplicationIdtogether, so it threw even though the publisher was present, and the caller swallowed the error and fell back to the user name.What changed
MsixService.ExtractPublisherFromPathAsync— a publisher-only extractor that reads justIdentity/@Publisherand rejects a missing/empty value. It does not require an<Applications>element or packageName, so partial manifests work.CertGenerateCommand— when--manifestis named explicitly and no--publisheroverrides it, the publisher is resolved up front. If it can't be extracted, the command fails with a clear, actionable error (non-zero exit) instead of silently substituting the default:Could not extract the publisher from the manifest '<path>': <reason>. Fix the manifest's Identity Publisher attribute, or pass --publisher explicitly.--jsonmode: a single{"error": "..."}document on stdout.--manifest) stay lenient and still fall back, which is the appropriate behavior when no source was explicitly requested.--manifestbullet now describes the fail-loud behavior.The user-name fallback remains reasonable when no publisher source was given at all; it is only removed for the case where the caller explicitly named a manifest.
Behavior
--manifestwith valid publisher, no<Applications>--manifestwith no/empty publisher--manifest…--json, extraction fails{"error":...}, exit 1--manifest, auto-discovered project manifest failsTesting
dotnet build src/winapp-CLI/winapp.sln -c Debug— succeeds, 0 warnings.MsixServicePackagingTests): nonexistent file, partial manifest returns publisher, missing publisher throws, empty/whitespace publisher throws.CertGenerateCommandTests): explicit manifest missing publisher (text +--json, asserting a single JSON document), partial manifest succeeds with the correct subject.No command/option surface changed, so
docs/cli-schema.jsonand the npm command types are unaffected.