From 07f0e62b490105d93cf10b1eeec3bdd2298775ba Mon Sep 17 00:00:00 2001 From: Zachary Teutsch Date: Sat, 19 Sep 2026 17:22:03 -0400 Subject: [PATCH 1/2] Fix cert generate --manifest silently falling back to user name (#839) `winapp cert generate --manifest ` 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 `` 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> --- docs/usage.md | 2 +- .../CertGenerateCommandTests.cs | 75 +++++++++++++++++++ .../MsixServicePackagingTests.cs | 74 ++++++++++++++++++ .../Commands/CertGenerateCommand.cs | 24 ++++++ .../WinApp.Cli/Services/MsixService.cs | 33 ++++++++ 5 files changed, 207 insertions(+), 1 deletion(-) diff --git a/docs/usage.md b/docs/usage.md index 2231c6d56..74f5bd385 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -1356,7 +1356,7 @@ winapp cert generate [options] **Options:** -- `--manifest ` - Extract publisher information from Package.appxmanifest +- `--manifest ` - Extract the certificate publisher from the manifest's `Identity/@Publisher`. Only the publisher is required, so a partially-complete manifest still works. If the manifest has no usable publisher, the command fails instead of substituting a default, so the certificate can never silently mismatch the manifest. - `--publisher ` - Publisher for the certificate. Accepts a full X.500 distinguished name (e.g., `CN=Contoso, O=Contoso Ltd, C=US`) or a bare name which is automatically wrapped as `CN=` - `--output ` - Output certificate file path (supports absolute and relative paths) - `--password ` - Certificate password (default: "password") diff --git a/src/winapp-CLI/WinApp.Cli.Tests/CertGenerateCommandTests.cs b/src/winapp-CLI/WinApp.Cli.Tests/CertGenerateCommandTests.cs index e11688d31..ac8a4f738 100644 --- a/src/winapp-CLI/WinApp.Cli.Tests/CertGenerateCommandTests.cs +++ b/src/winapp-CLI/WinApp.Cli.Tests/CertGenerateCommandTests.cs @@ -43,6 +43,55 @@ public async Task EmptyPassword_NonJson_ReturnsError() Assert.IsFalse(File.Exists(pfxPath), "No certificate should be created for an empty password."); } + // ── explicit --manifest publisher resolution (issue #839) ─────────── + + [TestMethod] + public async Task ExplicitManifest_MissingPublisher_NonJson_ReturnsError() + { + // A named --manifest that has no usable Identity/@Publisher must fail with an actionable + // error instead of silently falling back to the OS user name (issue #839). + var command = GetRequiredService(); + var manifestPath = Path.Combine(_tempDirectory.FullName, "NoPublisher.appxmanifest"); + await File.WriteAllTextAsync(manifestPath, """ + + + + + """); + var pfxPath = Path.Combine(_tempDirectory.FullName, "no-publisher.pfx"); + + var exitCode = await ParseAndInvokeWithCaptureAsync( + command, ["--manifest", manifestPath, "--output", pfxPath, "--password", "testpw"]); + + Assert.AreEqual(1, exitCode); + StringAssert.Contains(ConsoleStdErr.ToString(), "Could not extract the publisher from the manifest"); + Assert.IsFalse(File.Exists(pfxPath), "No certificate should be created when the manifest has no publisher."); + } + + [TestMethod] + public async Task ExplicitManifest_PublisherOnly_UsesManifestPublisher() + { + // A partially-complete manifest (valid Identity/@Publisher, no Applications element) is common + // mid-development. The certificate must use that publisher, not the OS user name (issue #839). + var command = GetRequiredService(); + var manifestPath = Path.Combine(_tempDirectory.FullName, "Partial.appxmanifest"); + await File.WriteAllTextAsync(manifestPath, """ + + + + + """); + var pfxPath = Path.Combine(_tempDirectory.FullName, "partial.pfx"); + + var exitCode = await ParseAndInvokeWithCaptureAsync( + command, ["--manifest", manifestPath, "--output", pfxPath, "--password", "testpw"]); + + Assert.AreEqual(0, exitCode, "A manifest with a valid publisher but no Applications element must still succeed."); + Assert.IsTrue(File.Exists(pfxPath), "The certificate should be created."); + using var cert = System.Security.Cryptography.X509Certificates.X509CertificateLoader.LoadPkcs12FromFile(pfxPath, "testpw"); + StringAssert.Contains(cert.Subject, "FlowHarnessPublisher"); + } + [TestMethod] public void OutputOption_AcceptsPlainFileName() { @@ -425,4 +474,30 @@ public async Task EmptyPassword_Json_OutputsJsonError() StringAssert.Contains(errorProp.GetString(), "password cannot be empty"); Assert.IsFalse(File.Exists(pfxPath)); } + + [TestMethod] + public async Task ExplicitManifest_MissingPublisher_Json_OutputsSingleJsonError() + { + // The VS Code extension passes --manifest with --json. A manifest that can't yield a publisher + // must produce exactly one structured error document — not empty stdout, and not two documents + // (issue #839). JsonDocument.Parse over the full output verifies it is a single JSON document. + var command = GetRequiredService(); + var manifestPath = Path.Combine(_tempDirectory.FullName, "NoPublisher.appxmanifest"); + await File.WriteAllTextAsync(manifestPath, """ + + + + + """); + var pfxPath = Path.Combine(_tempDirectory.FullName, "no-publisher-json.pfx"); + + var exitCode = await ParseAndInvokeWithCaptureAsync( + command, ["--manifest", manifestPath, "--output", pfxPath, "--password", "testpw", "--json"]); + + Assert.AreEqual(1, exitCode); + var root = JsonDocument.Parse(TestAnsiConsole.Output.Trim()).RootElement; + Assert.IsTrue(root.TryGetProperty("error", out var errorProp), "JSON error output should contain 'error' property"); + StringAssert.Contains(errorProp.GetString(), "Could not extract the publisher from the manifest"); + Assert.IsFalse(File.Exists(pfxPath)); + } } diff --git a/src/winapp-CLI/WinApp.Cli.Tests/MsixServicePackagingTests.cs b/src/winapp-CLI/WinApp.Cli.Tests/MsixServicePackagingTests.cs index 590b9eda8..85c010cc0 100644 --- a/src/winapp-CLI/WinApp.Cli.Tests/MsixServicePackagingTests.cs +++ b/src/winapp-CLI/WinApp.Cli.Tests/MsixServicePackagingTests.cs @@ -89,6 +89,80 @@ public async Task ParseAppxManifestFromPathAsync_ValidManifest_ReturnsIdentity() Assert.AreEqual("MyApp", result.ApplicationId); } + // ---- ExtractPublisherFromPathAsync -------------------------------------------- + + [TestMethod] + public async Task ExtractPublisherFromPathAsync_NonexistentFile_ThrowsFileNotFoundException() + { + var missing = new FileInfo(Path.Combine(_tempDirectory.FullName, "does-not-exist.xml")); + + var ex = await Assert.ThrowsExactlyAsync( + () => MsixService.ExtractPublisherFromPathAsync(missing, TestContext.CancellationToken)); + + StringAssert.Contains(ex.Message, "AppX manifest not found"); + } + + [TestMethod] + public async Task ExtractPublisherFromPathAsync_ManifestWithoutApplications_ReturnsPublisher() + { + // A partially-complete manifest (valid Identity/@Publisher, no Applications/Name) must still + // yield the publisher — unlike ParseAppxManifestFromPathAsync, which requires an Application + // element (issue #839). + const string manifest = + """ + + + + + """; + var manifestPath = new FileInfo(Path.Combine(_tempDirectory.FullName, "Partial.appxmanifest")); + await File.WriteAllTextAsync(manifestPath.FullName, manifest, TestContext.CancellationToken); + + var publisher = await MsixService.ExtractPublisherFromPathAsync(manifestPath, TestContext.CancellationToken); + + Assert.AreEqual("CN=FlowHarnessPublisher, O=Fabrikam Inc, C=US", publisher); + } + + [TestMethod] + public async Task ExtractPublisherFromPathAsync_MissingPublisher_ThrowsInvalidOperationException() + { + const string manifest = + """ + + + + + """; + var manifestPath = new FileInfo(Path.Combine(_tempDirectory.FullName, "NoPublisher.appxmanifest")); + await File.WriteAllTextAsync(manifestPath.FullName, manifest, TestContext.CancellationToken); + + var ex = await Assert.ThrowsExactlyAsync( + () => MsixService.ExtractPublisherFromPathAsync(manifestPath, TestContext.CancellationToken)); + + StringAssert.Contains(ex.Message, "Publisher"); + } + + [TestMethod] + public async Task ExtractPublisherFromPathAsync_EmptyPublisher_ThrowsInvalidOperationException() + { + // An empty/whitespace Publisher would otherwise slip through and surface a generic downstream + // "Publisher name cannot be empty" error rather than the actionable manifest guidance. + const string manifest = + """ + + + + + """; + var manifestPath = new FileInfo(Path.Combine(_tempDirectory.FullName, "EmptyPublisher.appxmanifest")); + await File.WriteAllTextAsync(manifestPath.FullName, manifest, TestContext.CancellationToken); + + var ex = await Assert.ThrowsExactlyAsync( + () => MsixService.ExtractPublisherFromPathAsync(manifestPath, TestContext.CancellationToken)); + + StringAssert.Contains(ex.Message, "Publisher"); + } + // ---- CopyManifestReferencedFiles ---------------------------------------------- [TestMethod] diff --git a/src/winapp-CLI/WinApp.Cli/Commands/CertGenerateCommand.cs b/src/winapp-CLI/WinApp.Cli/Commands/CertGenerateCommand.cs index be8114037..5e4ae75ca 100644 --- a/src/winapp-CLI/WinApp.Cli/Commands/CertGenerateCommand.cs +++ b/src/winapp-CLI/WinApp.Cli/Commands/CertGenerateCommand.cs @@ -131,6 +131,30 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio return 1; } + // When --manifest is named explicitly (and no --publisher overrides it), the caller is + // asking the certificate to match that manifest's Identity/@Publisher. Resolve it up front + // so a manifest that can't yield a publisher fails with a clear error here — before the + // status task — instead of silently falling back to the system default and producing a + // certificate that can never match the manifest (issue #839). + if (manifestPath != null && string.IsNullOrWhiteSpace(publisher)) + { + try + { + publisher = await MsixService.ExtractPublisherFromPathAsync(manifestPath, cancellationToken); + } + catch (Exception ex) + { + var message = $"Could not extract the publisher from the manifest '{manifestPath}': {ex.Message}. " + + "Fix the manifest's Identity Publisher attribute, or pass --publisher explicitly."; + if (json) + { + return JsonErrorOutput.Write(ansiConsole, message); + } + logger.LogError("{UISymbol} {Message}", UiSymbols.Error, message); + return 1; + } + } + CertificateService.CertificateResult? certResult = null; var returnCode = await statusService.ExecuteWithStatusAsync("Generating development certificate...", async (taskContext, ct) => diff --git a/src/winapp-CLI/WinApp.Cli/Services/MsixService.cs b/src/winapp-CLI/WinApp.Cli/Services/MsixService.cs index 087b1b448..1d0225a2b 100644 --- a/src/winapp-CLI/WinApp.Cli/Services/MsixService.cs +++ b/src/winapp-CLI/WinApp.Cli/Services/MsixService.cs @@ -51,6 +51,39 @@ public static async Task ParseAppxManifestFromPathAsync(File return ParseAppxManifestAsync(appxManifestContent); } + /// + /// Extracts only the Identity/@Publisher value from an AppX manifest file. + /// Unlike , this does not require the manifest to + /// declare a package Name or an Application element, so it succeeds on the partially-complete + /// manifests that are common mid-development, as long as a publisher is present. + /// + /// Path to the appxmanifest.xml file + /// Cancellation token + /// The publisher distinguished name from Identity/@Publisher + /// Thrown when the manifest file is not found + /// Thrown when the manifest is invalid or missing the Identity Publisher attribute + public static async Task ExtractPublisherFromPathAsync(FileInfo appxManifestPath, CancellationToken cancellationToken = default) + { + if (!appxManifestPath.Exists) + { + throw new FileNotFoundException($"AppX manifest not found at: {appxManifestPath}"); + } + + var appxManifestContent = await File.ReadAllTextAsync(appxManifestPath.FullName, Encoding.UTF8, cancellationToken); + var doc = AppxManifestDocument.Parse(appxManifestContent); + + var identity = doc.GetIdentityElement() + ?? throw new InvalidOperationException("No Identity element found in AppX manifest"); + + var publisher = identity.Attribute("Publisher")?.Value; + if (string.IsNullOrWhiteSpace(publisher)) + { + throw new InvalidOperationException("AppX manifest Identity element is missing the required Publisher attribute"); + } + + return publisher; + } + /// /// Parses an AppX manifest content and extracts the package identity information /// From 2d115f7fa8bbe9f387bc4da6f7e2e56ab4642891 Mon Sep 17 00:00:00 2001 From: Zachary Teutsch Date: Sat, 19 Sep 2026 17:48:06 -0400 Subject: [PATCH 2/2] Address PR review: propagate cancellation, use Path.Join in tests - 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> --- .../CertGenerateCommandTests.cs | 39 ++++++++++++++++--- .../MsixServicePackagingTests.cs | 8 ++-- .../Commands/CertGenerateCommand.cs | 2 +- 3 files changed, 38 insertions(+), 11 deletions(-) diff --git a/src/winapp-CLI/WinApp.Cli.Tests/CertGenerateCommandTests.cs b/src/winapp-CLI/WinApp.Cli.Tests/CertGenerateCommandTests.cs index ac8a4f738..c3ad0ebba 100644 --- a/src/winapp-CLI/WinApp.Cli.Tests/CertGenerateCommandTests.cs +++ b/src/winapp-CLI/WinApp.Cli.Tests/CertGenerateCommandTests.cs @@ -51,14 +51,14 @@ public async Task ExplicitManifest_MissingPublisher_NonJson_ReturnsError() // A named --manifest that has no usable Identity/@Publisher must fail with an actionable // error instead of silently falling back to the OS user name (issue #839). var command = GetRequiredService(); - var manifestPath = Path.Combine(_tempDirectory.FullName, "NoPublisher.appxmanifest"); + var manifestPath = Path.Join(_tempDirectory.FullName, "NoPublisher.appxmanifest"); await File.WriteAllTextAsync(manifestPath, """ """); - var pfxPath = Path.Combine(_tempDirectory.FullName, "no-publisher.pfx"); + var pfxPath = Path.Join(_tempDirectory.FullName, "no-publisher.pfx"); var exitCode = await ParseAndInvokeWithCaptureAsync( command, ["--manifest", manifestPath, "--output", pfxPath, "--password", "testpw"]); @@ -74,14 +74,14 @@ public async Task ExplicitManifest_PublisherOnly_UsesManifestPublisher() // A partially-complete manifest (valid Identity/@Publisher, no Applications element) is common // mid-development. The certificate must use that publisher, not the OS user name (issue #839). var command = GetRequiredService(); - var manifestPath = Path.Combine(_tempDirectory.FullName, "Partial.appxmanifest"); + var manifestPath = Path.Join(_tempDirectory.FullName, "Partial.appxmanifest"); await File.WriteAllTextAsync(manifestPath, """ """); - var pfxPath = Path.Combine(_tempDirectory.FullName, "partial.pfx"); + var pfxPath = Path.Join(_tempDirectory.FullName, "partial.pfx"); var exitCode = await ParseAndInvokeWithCaptureAsync( command, ["--manifest", manifestPath, "--output", pfxPath, "--password", "testpw"]); @@ -92,6 +92,33 @@ await File.WriteAllTextAsync(manifestPath, """ StringAssert.Contains(cert.Subject, "FlowHarnessPublisher"); } + [TestMethod] + public async Task ExplicitManifest_Canceled_DoesNotReportAsManifestError() + { + // Cancellation during manifest reading must not be translated into a "Could not extract the + // publisher" input error by the broad catch around extraction — that would misreport a + // cancellation as bad user input (especially for manifests on slow/unavailable paths). + var command = GetRequiredService(); + var manifestPath = Path.Join(_tempDirectory.FullName, "Cancel.appxmanifest"); + await File.WriteAllTextAsync(manifestPath, """ + + + + + """); + var pfxPath = Path.Join(_tempDirectory.FullName, "cancel.pfx"); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + var exitCode = await ParseAndInvokeWithCaptureAsync( + command, ["--manifest", manifestPath, "--output", pfxPath, "--password", "testpw"], cts.Token); + + Assert.AreNotEqual(0, exitCode, "A canceled operation must not report success."); + StringAssert.DoesNotMatch(ConsoleStdErr.ToString(), new System.Text.RegularExpressions.Regex("Could not extract the publisher from the manifest"), + "Cancellation must not be misreported as a manifest extraction error."); + Assert.IsFalse(File.Exists(pfxPath), "No certificate should be created when the operation is canceled."); + } + [TestMethod] public void OutputOption_AcceptsPlainFileName() { @@ -482,14 +509,14 @@ public async Task ExplicitManifest_MissingPublisher_Json_OutputsSingleJsonError( // must produce exactly one structured error document — not empty stdout, and not two documents // (issue #839). JsonDocument.Parse over the full output verifies it is a single JSON document. var command = GetRequiredService(); - var manifestPath = Path.Combine(_tempDirectory.FullName, "NoPublisher.appxmanifest"); + var manifestPath = Path.Join(_tempDirectory.FullName, "NoPublisher.appxmanifest"); await File.WriteAllTextAsync(manifestPath, """ """); - var pfxPath = Path.Combine(_tempDirectory.FullName, "no-publisher-json.pfx"); + var pfxPath = Path.Join(_tempDirectory.FullName, "no-publisher-json.pfx"); var exitCode = await ParseAndInvokeWithCaptureAsync( command, ["--manifest", manifestPath, "--output", pfxPath, "--password", "testpw", "--json"]); diff --git a/src/winapp-CLI/WinApp.Cli.Tests/MsixServicePackagingTests.cs b/src/winapp-CLI/WinApp.Cli.Tests/MsixServicePackagingTests.cs index 85c010cc0..e26e7d030 100644 --- a/src/winapp-CLI/WinApp.Cli.Tests/MsixServicePackagingTests.cs +++ b/src/winapp-CLI/WinApp.Cli.Tests/MsixServicePackagingTests.cs @@ -94,7 +94,7 @@ public async Task ParseAppxManifestFromPathAsync_ValidManifest_ReturnsIdentity() [TestMethod] public async Task ExtractPublisherFromPathAsync_NonexistentFile_ThrowsFileNotFoundException() { - var missing = new FileInfo(Path.Combine(_tempDirectory.FullName, "does-not-exist.xml")); + var missing = new FileInfo(Path.Join(_tempDirectory.FullName, "does-not-exist.xml")); var ex = await Assert.ThrowsExactlyAsync( () => MsixService.ExtractPublisherFromPathAsync(missing, TestContext.CancellationToken)); @@ -115,7 +115,7 @@ public async Task ExtractPublisherFromPathAsync_ManifestWithoutApplications_Retu """; - var manifestPath = new FileInfo(Path.Combine(_tempDirectory.FullName, "Partial.appxmanifest")); + var manifestPath = new FileInfo(Path.Join(_tempDirectory.FullName, "Partial.appxmanifest")); await File.WriteAllTextAsync(manifestPath.FullName, manifest, TestContext.CancellationToken); var publisher = await MsixService.ExtractPublisherFromPathAsync(manifestPath, TestContext.CancellationToken); @@ -133,7 +133,7 @@ public async Task ExtractPublisherFromPathAsync_MissingPublisher_ThrowsInvalidOp """; - var manifestPath = new FileInfo(Path.Combine(_tempDirectory.FullName, "NoPublisher.appxmanifest")); + var manifestPath = new FileInfo(Path.Join(_tempDirectory.FullName, "NoPublisher.appxmanifest")); await File.WriteAllTextAsync(manifestPath.FullName, manifest, TestContext.CancellationToken); var ex = await Assert.ThrowsExactlyAsync( @@ -154,7 +154,7 @@ public async Task ExtractPublisherFromPathAsync_EmptyPublisher_ThrowsInvalidOper """; - var manifestPath = new FileInfo(Path.Combine(_tempDirectory.FullName, "EmptyPublisher.appxmanifest")); + var manifestPath = new FileInfo(Path.Join(_tempDirectory.FullName, "EmptyPublisher.appxmanifest")); await File.WriteAllTextAsync(manifestPath.FullName, manifest, TestContext.CancellationToken); var ex = await Assert.ThrowsExactlyAsync( diff --git a/src/winapp-CLI/WinApp.Cli/Commands/CertGenerateCommand.cs b/src/winapp-CLI/WinApp.Cli/Commands/CertGenerateCommand.cs index 5e4ae75ca..cc03ffd32 100644 --- a/src/winapp-CLI/WinApp.Cli/Commands/CertGenerateCommand.cs +++ b/src/winapp-CLI/WinApp.Cli/Commands/CertGenerateCommand.cs @@ -142,7 +142,7 @@ public override async Task InvokeAsync(ParseResult parseResult, Cancellatio { publisher = await MsixService.ExtractPublisherFromPathAsync(manifestPath, cancellationToken); } - catch (Exception ex) + catch (Exception ex) when (ex is not OperationCanceledException) { var message = $"Could not extract the publisher from the manifest '{manifestPath}': {ex.Message}. " + "Fix the manifest's Identity Publisher attribute, or pass --publisher explicitly.";