From b0b6e0ee7d02e68b4ab361a230b2fb6574de2349 Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" Date: Wed, 12 Aug 2026 09:20:40 -0700 Subject: [PATCH 01/10] Add MSIXVC2 upload support to PackageUploader.exe via MakePkg.exe UploadXvcPackage now detects MSIXVC2 packages and delegates the upload to the MSIXVC2-capable MakePkg.exe, translating the operation config into MakePkg.exe command-line parameters. The legacy XVC1/MSIXVC1 upload path is unchanged. Delegation is gated strictly on positive MSIXVC2 package detection, because MakePkg.exe shells back out to PackageUploader.exe for XVC1 uploads and an unconditional delegation would create infinite process recursion. MSIXVC2 detection moves from PackageUploader.UI into PackageUploader.ClientApi/Packaging/PackageFormatDetector so the CLI and UI share one source of truth; XvcFile.IsLikelyMsixvc2Package now delegates to it. The MakePkg.exe capability check is a clearly marked placeholder in PackageUploader.Application/Tools that must be swapped for the shared IMsixvc2ToolResolver before merge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 24 +- .../UploadXvcPackageOperationConfigTest.cs | 35 +++ .../Tools/LoggerMockExtensions.cs | 27 ++ .../Tools/Msixvc2UploadArgumentBuilderTest.cs | 194 ++++++++++++ .../Tools/TempPackageFile.cs | 50 +++ .../UploadXvcPackageOperationMsixvc2Test.cs | 288 ++++++++++++++++++ .../Config/UploadXvcPackageOperationConfig.cs | 10 +- .../Extensions/HostExtensions.cs | 9 + .../Operations/UploadXvcPackageOperation.cs | 82 ++++- .../Properties/AssemblyInfo.cs | 5 +- .../Tools/IMsixvc2ProcessRunner.cs | 15 + .../Tools/IMsixvc2UploadToolProvider.cs | 23 ++ .../Tools/Msixvc2CapabilityPlaceholder.cs | 85 ++++++ .../Tools/Msixvc2CommandLineContext.cs | 14 + .../Tools/Msixvc2ProcessRunner.cs | 85 ++++++ .../Msixvc2UnsupportedOptionException.cs | 12 + .../Tools/Msixvc2UploadArgumentBuilder.cs | 204 +++++++++++++ .../Packaging/PackageFormatDetectorTest.cs | 114 +++++++ .../PackageUploaderExtensions.cs | 3 + .../Packaging/PackageFormatDetector.cs | 95 ++++++ src/PackageUploader.UI/Model/Xvc/XvcFile.cs | 78 +---- 21 files changed, 1373 insertions(+), 79 deletions(-) create mode 100644 src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs create mode 100644 src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs create mode 100644 src/PackageUploader.Application.Test/Tools/TempPackageFile.cs create mode 100644 src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs create mode 100644 src/PackageUploader.Application/Tools/IMsixvc2ProcessRunner.cs create mode 100644 src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs create mode 100644 src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs create mode 100644 src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs create mode 100644 src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs create mode 100644 src/PackageUploader.Application/Tools/Msixvc2UnsupportedOptionException.cs create mode 100644 src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs create mode 100644 src/PackageUploader.ClientApi.Test/Packaging/PackageFormatDetectorTest.cs create mode 100644 src/PackageUploader.ClientApi/Packaging/PackageFormatDetector.cs diff --git a/README.md b/README.md index 5b97273c..5beb5987 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,7 @@ This ReadMe covers the following: * [Service creation and authentication](#service-creation-and-authentication) * [Get Package Uploader](#get-package-uploader) * [Run Package Uploader](#run-package-uploader) +* [MSIXVC2 packages](#msixvc2-packages) * [Putting it all together](#putting-it-all-together) * [Example GetProduct operation](#example-getproduct-operation) * [Example UploadXvcPackage operation](#example-uploadxvcpackage-operation) @@ -176,13 +177,34 @@ The following table has important arguments for running Package Uploader. | **[GetProduct](https://github.com/microsoft/PackageUploader/blob/main/Operations.md#GetProduct)** | Gets metadata for the product. This is useful for getting the productId, BigId, and product name that's used in all configuration files. This also gets a list of the BranchFriendlyNames and FlightNames of the product. | | **[GetPackages](https://github.com/microsoft/PackageUploader/blob/main/Operations.md#GetPackages)** | Gets a list of the packages in a branch or flight. | | **[UploadUwpPackage](https://github.com/microsoft/PackageUploader/blob/main/Operations.md#UploadUwpPackage)** | Uploads a UWP game package. | -| **[UploadXvcPackage](https://github.com/microsoft/PackageUploader/blob/main/Operations.md#UploadXvcPackage)** | Uploads an XVC game package and assets, including EKB, SubVal, layout, and SODB files. | +| **[UploadXvcPackage](https://github.com/microsoft/PackageUploader/blob/main/Operations.md#UploadXvcPackage)** | Uploads an XVC game package and assets, including EKB, SubVal, layout, and SODB files. MSIXVC2 packages are detected automatically and uploaded through MakePkg.exe — see [MSIXVC2 packages](#msixvc2-packages). | | **[RemovePackages](https://github.com/microsoft/PackageUploader/blob/main/Operations.md#RemovePackages)** | Removes game packages and assets from a branch. We recommend keeping only your 10 most recent packages to ensure optimal performance. | | **[ImportPackages](https://github.com/microsoft/PackageUploader/blob/main/Operations.md#ImportPackages)** | Imports all game packages from a branch to a destination branch. Use this operation to copy your previously uploaded and published packages from one branch to another. | | **[PublishPackages](https://github.com/microsoft/PackageUploader/blob/main/Operations.md#PublishPackages)** | Publishes all game packages from a branch or flight to a destination sandbox or flight. You can set specific availability times in the configuration file. | For more information about operation parameters, see [Operations](https://github.com/microsoft/PackageUploader/blob/main/Operations.md). +### MSIXVC2 packages + +`UploadXvcPackage` detects the package format from the file you point `packageFilePath` at. When the package is an +MSIXVC2 package, PackageUploader delegates the upload to the MSIXVC2-capable `MakePkg.exe` that ships with the Microsoft +GDK, because MakePkg.exe owns the MSIXVC2 upload protocol. There is no separate operation name and no new configuration +switch — an XVC1/MSIXVC1 package continues to be uploaded by PackageUploader itself, exactly as before. + +The following configuration options behave differently on the MSIXVC2 path: + +| Option | Behavior | +| --- | --- | +| `gameAssets` | Not required. MakePkg.exe picks the assets up from the folder that contains the package. If the paths you supply resolve to that same folder they are ignored with a warning; if they point elsewhere the operation fails so an asset is never silently dropped. | +| `minutesToWaitForProcessing` | Ignored with a warning. MakePkg.exe manages its own processing wait. | +| `deltaUpload` | Ignored with a warning. | +| `availabilityDate` / `preDownloadDate` | Not supported. MakePkg.exe does not report back the identity of the package it uploaded, so the post-upload XVC configuration step cannot target it. Set these dates with a separate `PublishPackages` or configuration invocation. | +| `productId` | Supported. It is resolved to the corresponding Big ID, which is what MakePkg.exe requires. | +| Authentication | MakePkg.exe performs its own interactive, cached sign-in. Only `CacheableBrowser` is supported; other `--Authentication` values and `--TenantId` fail with an explicit error rather than being silently ignored. | + +If no MSIXVC2-capable `MakePkg.exe` is available, the operation fails with an actionable error instead of attempting an +upload that cannot succeed. + ### Available parameters | Parameter | Description | diff --git a/src/PackageUploader.Application.Test/Config/UploadXvcPackageOperationConfigTest.cs b/src/PackageUploader.Application.Test/Config/UploadXvcPackageOperationConfigTest.cs index 0665119d..c3a74eca 100644 --- a/src/PackageUploader.Application.Test/Config/UploadXvcPackageOperationConfigTest.cs +++ b/src/PackageUploader.Application.Test/Config/UploadXvcPackageOperationConfigTest.cs @@ -83,4 +83,39 @@ public void Validate_PreDownloadDateBeforeAvailabilityDate_NoPreDownloadError() Assert.DoesNotContain(r => r.MemberNames.Contains("PreDownloadDate"), results); } + + [TestMethod] + public void Validate_NonMsixvc2PackageWithoutGameAssets_ReturnsError() + { + var config = new TestUploadXvcPackageOperationConfig + { + OperationName = "UploadXvcPackage", + ProductId = "product-123", + BranchFriendlyName = "main", + PackageFilePath = "test.msixvc", + GameAssets = null, + }; + + var results = ConfigTestHelper.ValidateConfig(config); + + Assert.Contains(r => r.MemberNames.Contains("GameAssets"), results); + } + + [TestMethod] + public void Validate_Msixvc2PackageWithoutGameAssets_NoGameAssetsError() + { + using var package = Test.Tools.TempPackageFile.CreateMsixvc2(); + var config = new TestUploadXvcPackageOperationConfig + { + OperationName = "UploadXvcPackage", + ProductId = "product-123", + BranchFriendlyName = "main", + PackageFilePath = package.Path, + GameAssets = null, + }; + + var results = ConfigTestHelper.ValidateConfig(config); + + Assert.DoesNotContain(r => r.MemberNames.Contains("GameAssets"), results); + } } diff --git a/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs b/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs new file mode 100644 index 00000000..8412cc9a --- /dev/null +++ b/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs @@ -0,0 +1,27 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Logging; +using Moq; + +namespace PackageUploader.Application.Test.Tools; + +internal static class LoggerMockExtensions +{ + public static void VerifyLogErrorContains(this Mock> loggerMock, string expectedSubstring) => + VerifyLogContains(loggerMock, LogLevel.Error, expectedSubstring); + + public static void VerifyLogWarningContains(this Mock loggerMock, string expectedSubstring) => + VerifyLogContains(loggerMock, LogLevel.Warning, expectedSubstring); + + private static void VerifyLogContains(Mock loggerMock, LogLevel level, string expectedSubstring) + where TLogger : class, ILogger => + loggerMock.Verify( + x => x.Log( + level, + It.IsAny(), + It.Is((v, _) => v.ToString()!.Contains(expectedSubstring)), + It.IsAny(), + It.IsAny>()), + Times.Once); +} diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs new file mode 100644 index 00000000..49c94ce5 --- /dev/null +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs @@ -0,0 +1,194 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Logging; +using Moq; +using PackageUploader.Application.Config; +using PackageUploader.Application.Test.Config; +using PackageUploader.Application.Tools; +using PackageUploader.ClientApi; +using PackageUploader.ClientApi.Models; + +namespace PackageUploader.Application.Test.Tools; + +[TestClass] +public class Msixvc2UploadArgumentBuilderTest +{ + private const string BigId = "9NBLGGH4R315"; + private static readonly string PackageFilePath = Path.Combine(Path.GetTempPath(), "packages", "game.msixvc"); + private static readonly string PackageDirectory = Path.GetDirectoryName(Path.GetFullPath(PackageFilePath))!; + + private static readonly Msixvc2CommandLineContext CacheableBrowserContext = + new(IngestionExtensions.AuthenticationMethod.CacheableBrowser, TenantId: null); + + private readonly Mock _loggerMock = new(); + + private static UploadXvcPackageOperationConfig CreateConfig() => new TestUploadXvcPackageOperationConfig + { + OperationName = "UploadXvcPackage", + BigId = BigId, + BranchFriendlyName = "Main", + MarketGroupName = "default", + PackageFilePath = PackageFilePath, + }; + + private string Build(UploadXvcPackageOperationConfig config, Msixvc2CommandLineContext? context = null, bool supportsUploadSource = false) => + Msixvc2UploadArgumentBuilder.Build(config, context ?? CacheableBrowserContext, BigId, supportsUploadSource, _loggerMock.Object); + + [TestMethod] + public void Build_BranchConfig_ProducesExpectedArguments() + { + var args = Build(CreateConfig()); + + Assert.AreEqual( + $"upload /pd \"{PackageDirectory}\" /branch \"Main\" /market \"default\" /storeid \"{BigId}\" /auth CacheableBrowser", + args); + } + + [TestMethod] + public void Build_FlightConfigWithUploadSource_ProducesExpectedArguments() + { + var config = CreateConfig(); + config.BranchFriendlyName = null; + config.FlightName = "PreviewFlight"; + config.MarketGroupName = "NorthAmerica"; + + var args = Build(config, supportsUploadSource: true); + + Assert.AreEqual( + $"upload /pd \"{PackageDirectory}\" /flight \"PreviewFlight\" /market \"NorthAmerica\" /storeid \"{BigId}\" /uploadsource PackageUploader /auth CacheableBrowser", + args); + } + + /// + /// /msixvc2 belongs to the pack-from-content-folder form (Msixvc2UploadViewModel, which uses /d). + /// The already-built-package form used by the CLI mirrors PackageUploadViewModel and must not emit it. + /// + [TestMethod] + public void Build_DoesNotEmitMsixvc2FlagForAlreadyBuiltPackage() => + Assert.DoesNotContain("/msixvc2", Build(CreateConfig())); + + [TestMethod] + public void Build_ResolvedBigIdIsUsedInsteadOfConfigProductId() + { + var config = CreateConfig(); + config.BigId = null; + config.ProductId = "00000000-0000-0000-0000-000000000001"; + + var args = Msixvc2UploadArgumentBuilder.Build(config, CacheableBrowserContext, "9RESOLVED123", supportsUploadSource: false, _loggerMock.Object); + + StringAssert.Contains(args, "/storeid \"9RESOLVED123\""); + } + + [TestMethod] + public void Build_GameAssetsInPackageDirectory_WarnsAndContinues() + { + var config = CreateConfig(); + config.GameAssets = new GameAssets + { + EkbFilePath = Path.Combine(PackageDirectory, "game.ekb"), + SubValFilePath = Path.Combine(PackageDirectory, "validator.xml"), + }; + + var args = Build(config); + + StringAssert.Contains(args, "/storeid"); + _loggerMock.VerifyLogWarningContains("'gameAssets' is not used for MSIXVC2 uploads"); + } + + [TestMethod] + public void Build_GameAssetsOutsidePackageDirectory_Throws() + { + var strayPath = Path.Combine(Path.GetTempPath(), "elsewhere", "game.ekb"); + var config = CreateConfig(); + config.GameAssets = new GameAssets + { + EkbFilePath = strayPath, + SubValFilePath = Path.Combine(PackageDirectory, "validator.xml"), + }; + + var ex = Assert.ThrowsExactly(() => Build(config)); + + StringAssert.Contains(ex.Message, "gameAssets.ekbFilePath"); + StringAssert.Contains(ex.Message, strayPath); + StringAssert.Contains(ex.Message, PackageDirectory); + } + + [TestMethod] + public void Build_MinutesToWaitForProcessing_WarnsAndContinues() + { + var config = CreateConfig(); + config.MinutesToWaitForProcessing = 60; + + var args = Build(config); + + StringAssert.Contains(args, "/storeid"); + _loggerMock.VerifyLogWarningContains("'minutesToWaitForProcessing' is not used for MSIXVC2 uploads"); + } + + [TestMethod] + public void Build_DeltaUpload_WarnsAndContinues() + { + var config = CreateConfig(); + config.DeltaUpload = true; + + var args = Build(config); + + StringAssert.Contains(args, "/storeid"); + _loggerMock.VerifyLogWarningContains("'deltaUpload' is not used for MSIXVC2 uploads"); + } + + [TestMethod] + public void Build_AvailabilityDate_Throws() + { + var config = CreateConfig(); + config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(1) }; + + var ex = Assert.ThrowsExactly(() => Build(config)); + + StringAssert.Contains(ex.Message, "'availabilityDate' cannot be applied"); + } + + [TestMethod] + public void Build_DisabledAvailabilityDate_DoesNotThrow() + { + var config = CreateConfig(); + config.AvailabilityDate = new GamePackageDate { IsEnabled = false }; + + StringAssert.Contains(Build(config), "/storeid"); + } + + [TestMethod] + public void Build_PreDownloadDate_Throws() + { + var config = CreateConfig(); + config.PreDownloadDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(1) }; + + var ex = Assert.ThrowsExactly(() => Build(config)); + + StringAssert.Contains(ex.Message, "'preDownloadDate' cannot be applied"); + } + + [TestMethod] + public void Build_TenantId_Throws() + { + var ex = Assert.ThrowsExactly(() => Build( + CreateConfig(), + new Msixvc2CommandLineContext(IngestionExtensions.AuthenticationMethod.CacheableBrowser, "contoso.onmicrosoft.com"))); + + StringAssert.Contains(ex.Message, "TenantId"); + } + + [TestMethod] + [DataRow(IngestionExtensions.AuthenticationMethod.AppSecret)] + [DataRow(IngestionExtensions.AuthenticationMethod.Browser)] + [DataRow(IngestionExtensions.AuthenticationMethod.AzureCli)] + public void Build_NonCacheableBrowserAuthentication_Throws(IngestionExtensions.AuthenticationMethod method) + { + var ex = Assert.ThrowsExactly(() => Build( + CreateConfig(), + new Msixvc2CommandLineContext(method, TenantId: null))); + + StringAssert.Contains(ex.Message, method.ToString()); + } +} diff --git a/src/PackageUploader.Application.Test/Tools/TempPackageFile.cs b/src/PackageUploader.Application.Test/Tools/TempPackageFile.cs new file mode 100644 index 00000000..59dea67d --- /dev/null +++ b/src/PackageUploader.Application.Test/Tools/TempPackageFile.cs @@ -0,0 +1,50 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Text; + +namespace PackageUploader.Application.Test.Tools; + +/// +/// Creates throwaway package files on disk for MSIXVC2 detection tests. +/// +internal sealed class TempPackageFile : IDisposable +{ + private const int MinimumDetectableSize = 4096; + + public string Path { get; } + + private TempPackageFile(string path) => Path = path; + + /// Creates a .msixvc file that starts with the ZIP local file header, i.e. an MSIXVC2 package. + public static TempPackageFile CreateMsixvc2() => Create(".msixvc", [0x50, 0x4B, 0x03, 0x04]); + + /// Creates a .msixvc file with no ZIP signatures, i.e. a legacy MSIXVC1/XVC1 package. + public static TempPackageFile CreateLegacyXvc() => Create(".msixvc", Encoding.ASCII.GetBytes("msft")); + + private static TempPackageFile Create(string extension, byte[] header) + { + var path = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"pu-test-{Guid.NewGuid():N}{extension}"); + + var contents = new byte[MinimumDetectableSize * 2]; + Array.Copy(header, contents, header.Length); + File.WriteAllBytes(path, contents); + + return new TempPackageFile(path); + } + + public void Dispose() + { + try + { + if (File.Exists(Path)) + { + File.Delete(Path); + } + } + catch (IOException) + { + // Best effort cleanup. + } + } +} diff --git a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs new file mode 100644 index 00000000..063993f3 --- /dev/null +++ b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs @@ -0,0 +1,288 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Moq; +using PackageUploader.Application.Config; +using PackageUploader.Application.Operations; +using PackageUploader.Application.Test.Config; +using PackageUploader.Application.Tools; +using PackageUploader.ClientApi; +using PackageUploader.ClientApi.Client.Ingestion.Models; +using PackageUploader.ClientApi.Models; +using System.Runtime.CompilerServices; + +namespace PackageUploader.Application.Test.Tools; + +[TestClass] +public class UploadXvcPackageOperationMsixvc2Test +{ + private const string BigId = "9NBLGGH4R315"; + private const string ResolvedMakePkgPath = @"C:\GDK\bin\MakePkg.exe"; + + private readonly Mock _serviceMock = new(); + private readonly Mock> _loggerMock = new(); + private readonly Mock _toolProviderMock = new(); + private readonly Mock _processRunnerMock = new(); + + private UploadXvcPackageOperation CreateOperation( + UploadXvcPackageOperationConfig config, + IngestionExtensions.AuthenticationMethod authenticationMethod = IngestionExtensions.AuthenticationMethod.CacheableBrowser) => + new(_serviceMock.Object, + _loggerMock.Object, + Options.Create(config), + _toolProviderMock.Object, + _processRunnerMock.Object, + new Msixvc2CommandLineContext(authenticationMethod, TenantId: null)); + + private static UploadXvcPackageOperationConfig CreateConfig(string packageFilePath) => new TestUploadXvcPackageOperationConfig + { + OperationName = "UploadXvcPackage", + BigId = BigId, + BranchFriendlyName = "Main", + MarketGroupName = "default", + PackageFilePath = packageFilePath, + }; + + private static string ExpectedArguments(string packageFilePath) => + $"upload /pd \"{Path.GetDirectoryName(Path.GetFullPath(packageFilePath))}\" /branch \"Main\" /market \"default\" /storeid \"{BigId}\" /auth CacheableBrowser"; + + private static GameProduct CreateProduct(string productId, string bigId) + { + var product = (GameProduct)RuntimeHelpers.GetUninitializedObject(typeof(GameProduct)); + typeof(GameProduct).GetProperty("ProductId")!.SetValue(product, productId); + typeof(GameProduct).GetProperty("BigId")!.SetValue(product, bigId); + return product; + } + + private void SetUpAvailableTool(bool supportsUploadSource = false) + { + _toolProviderMock.SetupGet(x => x.IsAvailable).Returns(true); + _toolProviderMock.SetupGet(x => x.ExecutablePath).Returns(ResolvedMakePkgPath); + _toolProviderMock.SetupGet(x => x.SupportsUploadSource).Returns(supportsUploadSource); + } + + private void SetUpSuccessfulRun() => + _processRunnerMock + .Setup(x => x.RunAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(0); + + [TestMethod] + public async Task Msixvc2PackageWithCapability_ShellsOutWithExpectedArguments() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _processRunnerMock.Verify( + x => x.RunAsync(ResolvedMakePkgPath, ExpectedArguments(package.Path), It.IsAny()), + Times.Once); + _serviceMock.VerifyNoOtherCalls(); + } + + [TestMethod] + public async Task Msixvc2PackageWithProductIdOnly_ResolvesBigIdThroughIngestion() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + + var config = CreateConfig(package.Path); + config.BigId = null; + config.ProductId = "1234567890"; + + _serviceMock + .Setup(x => x.GetProductByProductIdAsync("1234567890", It.IsAny())) + .ReturnsAsync(CreateProduct("1234567890", BigId)); + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _processRunnerMock.Verify( + x => x.RunAsync(ResolvedMakePkgPath, ExpectedArguments(package.Path), It.IsAny()), + Times.Once); + } + + [TestMethod] + public async Task Msixvc2PackageWithUnresolvableProductId_FailsAndDoesNotShellOut() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + + var config = CreateConfig(package.Path); + config.BigId = null; + config.ProductId = "1234567890"; + + _serviceMock + .Setup(x => x.GetProductByProductIdAsync("1234567890", It.IsAny())) + .ReturnsAsync(CreateProduct("1234567890", bigId: null!)); + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _processRunnerMock.VerifyNoOtherCalls(); + _loggerMock.VerifyLogErrorContains("Could not resolve a Big ID"); + } + + [TestMethod] + public async Task Msixvc2PackageWithoutCapability_FailsAndDoesNotShellOut() + { + using var package = TempPackageFile.CreateMsixvc2(); + _toolProviderMock.SetupGet(x => x.IsAvailable).Returns(false); + _toolProviderMock.SetupGet(x => x.ExecutablePath).Returns((string?)null); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _processRunnerMock.VerifyNoOtherCalls(); + _loggerMock.VerifyLogErrorContains("no MSIXVC2-capable MakePkg.exe was found"); + } + + /// + /// Circular-dependency guard: MakePkg.exe shells back out to PackageUploader.exe for XVC1/MSIXVC1 + /// uploads, so a non-MSIXVC2 package must NEVER be delegated to MakePkg.exe. + /// + [TestMethod] + public async Task NonMsixvc2Package_NeverShellsOut() + { + using var package = TempPackageFile.CreateLegacyXvc(); + SetUpAvailableTool(); + _serviceMock + .Setup(x => x.GetProductByBigIdAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new InvalidOperationException("took the normal XVC upload path")); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _processRunnerMock.VerifyNoOtherCalls(); + _serviceMock.Verify(x => x.GetProductByBigIdAsync(BigId, It.IsAny()), Times.Once); + } + + /// + /// The legacy XVC path must be byte-for-byte identical: the same ingestion calls in the same order, + /// with the same arguments, and no MakePkg.exe involvement. + /// + [TestMethod] + public async Task NonMsixvc2Package_TakesUnchangedLegacyUploadPath() + { + using var package = TempPackageFile.CreateLegacyXvc(); + SetUpAvailableTool(); + + var config = CreateConfig(package.Path); + config.GameAssets = new GameAssets { EkbFilePath = "ekb", SubValFilePath = "sub" }; + + var product = CreateProduct("1234567890", BigId); + var branch = (GamePackageBranch)RuntimeHelpers.GetUninitializedObject(typeof(GamePackageBranch)); + var marketGroupPackage = (GameMarketGroupPackage)RuntimeHelpers.GetUninitializedObject(typeof(GameMarketGroupPackage)); + typeof(GameMarketGroupPackage).GetProperty("Name")!.SetValue(marketGroupPackage, "default"); + var packageConfiguration = (GamePackageConfiguration)RuntimeHelpers.GetUninitializedObject(typeof(GamePackageConfiguration)); + typeof(GamePackageConfiguration).GetProperty("MarketGroupPackages")!.SetValue(packageConfiguration, new List { marketGroupPackage }); + var gamePackage = (GamePackage)RuntimeHelpers.GetUninitializedObject(typeof(GamePackage)); + + _serviceMock.Setup(x => x.GetProductByBigIdAsync(BigId, It.IsAny())).ReturnsAsync(product); + _serviceMock.Setup(x => x.GetPackageBranchByFriendlyNameAsync(product, "Main", It.IsAny())).ReturnsAsync(branch); + _serviceMock.Setup(x => x.GetPackageConfigurationAsync(product, branch, It.IsAny())).ReturnsAsync(packageConfiguration); + _serviceMock + .Setup(x => x.UploadGamePackageAsync(product, branch, marketGroupPackage, package.Path, config.GameAssets, 30, false, true, It.IsAny())) + .ReturnsAsync(gamePackage); + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _processRunnerMock.VerifyNoOtherCalls(); + _serviceMock.Verify( + x => x.UploadGamePackageAsync(product, branch, marketGroupPackage, package.Path, config.GameAssets, 30, false, true, It.IsAny()), + Times.Once); + _serviceMock.Verify( + x => x.SetXvcConfigurationAsync(It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + [TestMethod] + public async Task MissingPackageFile_NeverShellsOut() + { + SetUpAvailableTool(); + _serviceMock + .Setup(x => x.GetProductByBigIdAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new InvalidOperationException("took the normal XVC upload path")); + + var result = await CreateOperation(CreateConfig(@"C:\does\not\exist.msixvc")).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _processRunnerMock.VerifyNoOtherCalls(); + } + + [TestMethod] + public async Task UnsupportedConfigOption_FailsWithExplicitErrorAndDoesNotShellOut() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + + var config = CreateConfig(package.Path); + config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(1) }; + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _processRunnerMock.VerifyNoOtherCalls(); + _loggerMock.VerifyLogErrorContains("'availabilityDate' cannot be applied"); + } + + [TestMethod] + public async Task IgnorableConfigOption_WarnsAndStillShellsOut() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + + var config = CreateConfig(package.Path); + config.DeltaUpload = true; + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _processRunnerMock.Verify( + x => x.RunAsync(ResolvedMakePkgPath, ExpectedArguments(package.Path), It.IsAny()), + Times.Once); + } + + [TestMethod] + public async Task NonZeroExitCode_FailsOperation() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + _processRunnerMock + .Setup(x => x.RunAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(7); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _loggerMock.VerifyLogErrorContains("MakePkg.exe failed with exit code 7"); + } + + [TestMethod] + public async Task Cancellation_PropagatesTokenAndFailsOperation() + { + using var package = TempPackageFile.CreateMsixvc2(); + using var cts = new CancellationTokenSource(); + SetUpAvailableTool(); + + var observedToken = CancellationToken.None; + _processRunnerMock + .Setup(x => x.RunAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .Callback((_, _, ct) => observedToken = ct) + .ThrowsAsync(new OperationCanceledException()); + + await cts.CancelAsync(); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(cts.Token); + + Assert.AreEqual(1, result); + Assert.IsTrue(observedToken.IsCancellationRequested, "The operation cancellation token must be handed to the process runner."); + } +} diff --git a/src/PackageUploader.Application/Config/UploadXvcPackageOperationConfig.cs b/src/PackageUploader.Application/Config/UploadXvcPackageOperationConfig.cs index e5bc1d8e..86c21eff 100644 --- a/src/PackageUploader.Application/Config/UploadXvcPackageOperationConfig.cs +++ b/src/PackageUploader.Application/Config/UploadXvcPackageOperationConfig.cs @@ -3,6 +3,7 @@ using Microsoft.Extensions.Options; using PackageUploader.ClientApi.Models; +using PackageUploader.ClientApi.Packaging; using System.Collections.Generic; using System.ComponentModel.DataAnnotations; @@ -15,7 +16,9 @@ internal class UploadXvcPackageOperationConfig : UploadPackageOperationConfig, I { internal override string GetOperationName() => "UploadXvcPackage"; - [Required] + // Not [Required] at the attribute level: MSIXVC2 packages are uploaded by MakePkg.exe, which has no + // concept of EKB/submission-validator assets. Requiredness is enforced in Validate() for every + // non-MSIXVC2 package, so XVC1 behaviour is unchanged. [ValidateObjectMembers] public GameAssets GameAssets { get; set; } @@ -28,6 +31,11 @@ internal class UploadXvcPackageOperationConfig : UploadPackageOperationConfig, I foreach (var validationResult in base.Validate(validationContext)) yield return validationResult; + if (GameAssets is null && !PackageFormatDetector.IsLikelyMsixvc2Package(PackageFilePath)) + { + yield return new ValidationResult($"The {nameof(GameAssets)} field is required.", [nameof(GameAssets)]); + } + if (PreDownloadDate is { IsEnabled: true, EffectiveDate: null }) { yield return new ValidationResult($"If {nameof(PreDownloadDate)} {nameof(PreDownloadDate.IsEnabled)} is true, {nameof(PreDownloadDate.EffectiveDate)} needs to be set.", [nameof(PreDownloadDate)]); diff --git a/src/PackageUploader.Application/Extensions/HostExtensions.cs b/src/PackageUploader.Application/Extensions/HostExtensions.cs index 4e4c9c22..c1694fd1 100644 --- a/src/PackageUploader.Application/Extensions/HostExtensions.cs +++ b/src/PackageUploader.Application/Extensions/HostExtensions.cs @@ -8,6 +8,7 @@ using Microsoft.Extensions.Options; using PackageUploader.Application.Config; using PackageUploader.Application.Operations; +using PackageUploader.Application.Tools; using PackageUploader.ClientApi; using PackageUploader.ClientApi.Client.Ingestion.TokenProvider.Models; using PackageUploader.FileLogger; @@ -73,6 +74,14 @@ internal static HostApplicationBuilder ConfigureServices(this HostApplicationBui hostAppBuilder.Services.AddSingleton(new DataOutputOptions(isData)); hostAppBuilder.Services.AddPackageUploaderService(parseResult.GetValue(CommandLineHelper.AuthenticationMethodOption)); + hostAppBuilder.Services.AddSingleton(new Msixvc2CommandLineContext( + parseResult.GetValue(CommandLineHelper.AuthenticationMethodOption), + parseResult.GetValue(CommandLineHelper.TenantIdOption))); + hostAppBuilder.Services.AddSingleton(); + // TODO(GDK-release): swap Msixvc2CapabilityPlaceholder for an adapter over + // PackageUploader.ClientApi.Tools.IMsixvc2ToolResolver once PR # merges. + hostAppBuilder.Services.AddSingleton(); + hostAppBuilder.Services .AddScoped() .AddSingleton, GetProductOperationValidator>() diff --git a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs index aad9118e..34bd4ba4 100644 --- a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs +++ b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs @@ -5,23 +5,44 @@ using Microsoft.Extensions.Options; using PackageUploader.Application.Config; using PackageUploader.Application.Extensions; +using PackageUploader.Application.Tools; using PackageUploader.ClientApi; +using PackageUploader.ClientApi.Packaging; using System; using System.Threading; using System.Threading.Tasks; namespace PackageUploader.Application.Operations; -internal class UploadXvcPackageOperation(IPackageUploaderService storeBrokerService, ILogger logger, IOptions config) : Operation(logger) +internal class UploadXvcPackageOperation( + IPackageUploaderService storeBrokerService, + ILogger logger, + IOptions config, + IMsixvc2UploadToolProvider msixvc2ToolProvider, + IMsixvc2ProcessRunner msixvc2ProcessRunner, + Msixvc2CommandLineContext msixvc2CommandLineContext) : Operation(logger) { private readonly IPackageUploaderService _storeBrokerService = storeBrokerService ?? throw new ArgumentNullException(nameof(storeBrokerService)); private readonly ILogger _logger = logger ?? throw new ArgumentNullException(nameof(logger)); private readonly UploadXvcPackageOperationConfig _config = config?.Value ?? throw new ArgumentNullException(nameof(config)); + private readonly IMsixvc2UploadToolProvider _msixvc2ToolProvider = msixvc2ToolProvider ?? throw new ArgumentNullException(nameof(msixvc2ToolProvider)); + private readonly IMsixvc2ProcessRunner _msixvc2ProcessRunner = msixvc2ProcessRunner ?? throw new ArgumentNullException(nameof(msixvc2ProcessRunner)); + private readonly Msixvc2CommandLineContext _msixvc2CommandLineContext = msixvc2CommandLineContext ?? throw new ArgumentNullException(nameof(msixvc2CommandLineContext)); protected override async Task ProcessAsync(CancellationToken ct) { _logger.LogInformation("Starting {operationName} operation.", _config.GetOperationName()); + // SAFETY: only MSIXVC2 packages may be delegated to MakePkg.exe. MakePkg.exe shells back out to + // PackageUploader.exe for XVC1/MSIXVC1 uploads, so delegating any other package format here would + // create an infinite process recursion between the two executables. This guard is deliberately the + // package-format detection itself (not a config flag) so it cannot be bypassed by configuration. + if (PackageFormatDetector.IsLikelyMsixvc2Package(_config.PackageFilePath)) + { + await UploadMsixvc2PackageAsync(ct).ConfigureAwait(false); + return; + } + var product = await _storeBrokerService.GetProductAsync(_config, ct).ConfigureAwait(false); var packageBranch = await _storeBrokerService.GetGamePackageBranch(product, _config, ct).ConfigureAwait(false); var marketGroupPackage = await _storeBrokerService.GetGameMarketGroupPackage(product, packageBranch, _config, ct).ConfigureAwait(false); @@ -36,4 +57,61 @@ protected override async Task ProcessAsync(CancellationToken ct) _logger.LogInformation("Configuration set for Xvc packages"); } } -} \ No newline at end of file + + /// + /// Delegates the upload of an MSIXVC2 package to MakePkg.exe, which owns the MSIXVC2 upload protocol. + /// Only ever reached when the package has been positively identified as MSIXVC2. + /// + private async Task UploadMsixvc2PackageAsync(CancellationToken ct) + { + _logger.LogInformation("MSIXVC2 package detected. Delegating the upload to MakePkg.exe."); + + if (!_msixvc2ToolProvider.IsAvailable || string.IsNullOrWhiteSpace(_msixvc2ToolProvider.ExecutablePath)) + { + throw new InvalidOperationException( + "The package is an MSIXVC2 package, but no MSIXVC2-capable MakePkg.exe was found. " + + "Install the latest Microsoft GDK (or the Microsoft.Xbox.Packaging.Tools package) and try again."); + } + + var bigId = await ResolveBigIdAsync(ct).ConfigureAwait(false); + + var executablePath = _msixvc2ToolProvider.ExecutablePath; + var arguments = Msixvc2UploadArgumentBuilder.Build(_config, _msixvc2CommandLineContext, bigId, _msixvc2ToolProvider.SupportsUploadSource, _logger); + + _logger.LogInformation("Running {executablePath} {arguments}", executablePath, arguments); + + var exitCode = await _msixvc2ProcessRunner.RunAsync(executablePath, arguments, ct).ConfigureAwait(false); + + if (exitCode != 0) + { + throw new InvalidOperationException($"MakePkg.exe failed with exit code {exitCode}."); + } + + _logger.LogInformation("MSIXVC2 package uploaded successfully."); + } + + /// + /// MakePkg.exe identifies the product by Store ID (/storeid) only. When the config supplies a ProductId + /// instead, resolve it to the corresponding Big ID through the ingestion service rather than failing. + /// + private async Task ResolveBigIdAsync(CancellationToken ct) + { + if (!string.IsNullOrWhiteSpace(_config.BigId)) + { + return _config.BigId; + } + + _logger.LogInformation("Resolving Big ID for product {productId}, which MakePkg.exe requires for MSIXVC2 uploads.", _config.ProductId); + + var product = await _storeBrokerService.GetProductAsync(_config, ct).ConfigureAwait(false); + + if (string.IsNullOrWhiteSpace(product?.BigId)) + { + throw new InvalidOperationException( + $"Could not resolve a Big ID for product '{_config.ProductId}'. MakePkg.exe identifies products by Store ID for MSIXVC2 uploads; " + + "set 'bigId' in the config file or pass --BigId."); + } + + return product.BigId; + } +} diff --git a/src/PackageUploader.Application/Properties/AssemblyInfo.cs b/src/PackageUploader.Application/Properties/AssemblyInfo.cs index 34cc0624..28d91ecb 100644 --- a/src/PackageUploader.Application/Properties/AssemblyInfo.cs +++ b/src/PackageUploader.Application/Properties/AssemblyInfo.cs @@ -3,4 +3,7 @@ using System.Runtime.CompilerServices; -[assembly: InternalsVisibleTo("PackageUploader.Application.Test")] \ No newline at end of file +[assembly: InternalsVisibleTo("PackageUploader.Application.Test")] + +// Allows Moq to create proxies for internal interfaces (IMsixvc2UploadToolProvider, IMsixvc2ProcessRunner) in tests. +[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2")] \ No newline at end of file diff --git a/src/PackageUploader.Application/Tools/IMsixvc2ProcessRunner.cs b/src/PackageUploader.Application/Tools/IMsixvc2ProcessRunner.cs new file mode 100644 index 00000000..eb860aa1 --- /dev/null +++ b/src/PackageUploader.Application/Tools/IMsixvc2ProcessRunner.cs @@ -0,0 +1,15 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Threading; +using System.Threading.Tasks; + +namespace PackageUploader.Application.Tools; + +/// +/// Runs an external process, streaming its output, and returns its exit code. +/// +internal interface IMsixvc2ProcessRunner +{ + Task RunAsync(string executablePath, string arguments, CancellationToken ct); +} diff --git a/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs b/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs new file mode 100644 index 00000000..c83d27e3 --- /dev/null +++ b/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs @@ -0,0 +1,23 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#nullable enable + +namespace PackageUploader.Application.Tools; + +/// +/// Narrow abstraction over "is an MSIXVC2-capable MakePkg.exe available, and where is it?". +/// PackageUploader.Application depends only on this interface so that the underlying capability +/// resolution can be swapped without touching any consuming code. +/// +internal interface IMsixvc2UploadToolProvider +{ + /// True when an MSIXVC2-capable packaging tool is installed and usable. + bool IsAvailable { get; } + + /// Full path to the resolved MakePkg.exe (or makepkg2.exe fallback). Null when unavailable. + string? ExecutablePath { get; } + + /// True when the resolved tool understands the /uploadsource flag. + bool SupportsUploadSource { get; } +} diff --git a/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs b/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs new file mode 100644 index 00000000..78e240b5 --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs @@ -0,0 +1,85 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#nullable enable + +using Microsoft.Extensions.Logging; +using System; +using System.Diagnostics; + +namespace PackageUploader.Application.Tools; + +/// +/// PLACEHOLDER implementation of . +/// +/// TODO(GDK-release): replace with IMsixvc2ToolResolver from PackageUploader.ClientApi once PR #<change-1> merges. +/// That PR adds PackageUploader.ClientApi.Tools.IMsixvc2ToolResolver, which performs the real +/// MakePkg.exe (preferred) / makepkg2.exe (fallback) discovery and returns null when unavailable. +/// The rebase is intended to be a one-line change: re-point the IMsixvc2UploadToolProvider +/// registration in HostExtensions.ConfigureServices at an adapter over IMsixvc2ToolResolver. +/// Until then this placeholder reports the capability as always available and relies on +/// MakePkg.exe being resolvable from PATH. +/// +internal sealed class Msixvc2CapabilityPlaceholder(ILogger logger) : IMsixvc2UploadToolProvider +{ + private const string PlaceholderExecutable = "MakePkg.exe"; + private const int UploadSourceProbeTimeoutMs = 5000; + + private readonly ILogger _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + private bool? _supportsUploadSource; + + // TODO(GDK-release): replace with IMsixvc2ToolResolver.IsMsixvc2Supported(). + public bool IsAvailable => true; + + // TODO(GDK-release): replace with IMsixvc2ToolResolver.Resolve()?.ExecutablePath. + public string? ExecutablePath => PlaceholderExecutable; + + /// + /// Probes the tool for /uploadsource support, mirroring the UI's behaviour + /// (see Msixvc2UploadViewModel.SupportsUploadSourceFlag). Result is cached per process. + /// + public bool SupportsUploadSource => _supportsUploadSource ??= ProbeUploadSourceSupport(); + + private bool ProbeUploadSourceSupport() + { + var executablePath = ExecutablePath; + if (string.IsNullOrEmpty(executablePath)) + { + return false; + } + + try + { + using var process = new Process + { + StartInfo = new ProcessStartInfo + { + FileName = executablePath, + Arguments = "supports uploadsource", + UseShellExecute = false, + RedirectStandardOutput = false, + RedirectStandardError = false, + CreateNoWindow = true, + }, + }; + + process.Start(); + if (process.WaitForExit(UploadSourceProbeTimeoutMs)) + { + var supported = process.ExitCode == 0; + _logger.LogDebug("MakePkg /uploadsource probe: {Result} (exit code {ExitCode}).", + supported ? "supported" : "not supported", process.ExitCode); + return supported; + } + + try { process.Kill(entireProcessTree: true); } catch { /* best effort */ } + _logger.LogWarning("MakePkg /uploadsource probe timed out after {TimeoutMs}ms.", UploadSourceProbeTimeoutMs); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed to probe MakePkg for /uploadsource support."); + } + + return false; + } +} diff --git a/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs b/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs new file mode 100644 index 00000000..f6444146 --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs @@ -0,0 +1,14 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#nullable enable + +using PackageUploader.ClientApi; + +namespace PackageUploader.Application.Tools; + +/// +/// Command line values that are not part of the bound operation configuration but are still needed +/// to build MakePkg.exe arguments. +/// +internal sealed record Msixvc2CommandLineContext(IngestionExtensions.AuthenticationMethod AuthenticationMethod, string? TenantId); diff --git a/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs b/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs new file mode 100644 index 00000000..1c1b0048 --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs @@ -0,0 +1,85 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Logging; +using System; +using System.Diagnostics; +using System.Threading; +using System.Threading.Tasks; + +namespace PackageUploader.Application.Tools; + +/// +/// Drives MakePkg.exe the same way the UI does (see Msixvc2UploadingViewModel.RunMakePkg2ProcessAsync): +/// no shell execute, redirected stdout/stderr, no console window. Output is streamed through the +/// application logger so console users see live progress, and the child process is killed on cancellation. +/// +internal sealed class Msixvc2ProcessRunner(ILogger logger) : IMsixvc2ProcessRunner +{ + private readonly ILogger _logger = logger ?? throw new ArgumentNullException(nameof(logger)); + + public async Task RunAsync(string executablePath, string arguments, CancellationToken ct) + { + using var process = new Process + { + StartInfo = new ProcessStartInfo + { + FileName = executablePath, + Arguments = arguments, + UseShellExecute = false, + RedirectStandardOutput = true, + RedirectStandardError = true, + CreateNoWindow = true, + }, + EnableRaisingEvents = true, + }; + + process.OutputDataReceived += (_, e) => + { + if (!string.IsNullOrEmpty(e.Data)) + { + _logger.LogInformation("[MakePkg] {Data}", e.Data); + } + }; + + process.ErrorDataReceived += (_, e) => + { + if (!string.IsNullOrEmpty(e.Data)) + { + _logger.LogWarning("[MakePkg] {Data}", e.Data); + } + }; + + process.Start(); + process.BeginOutputReadLine(); + process.BeginErrorReadLine(); + + try + { + await process.WaitForExitAsync(ct).ConfigureAwait(false); + } + catch (OperationCanceledException) + { + KillProcess(process); + throw; + } + + return process.ExitCode; + } + + private void KillProcess(Process process) + { + try + { + if (!process.HasExited) + { + _logger.LogWarning("Cancellation requested. Terminating MakePkg.exe."); + process.Kill(entireProcessTree: true); + } + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Failed to terminate MakePkg.exe after cancellation."); + } + } +} diff --git a/src/PackageUploader.Application/Tools/Msixvc2UnsupportedOptionException.cs b/src/PackageUploader.Application/Tools/Msixvc2UnsupportedOptionException.cs new file mode 100644 index 00000000..75b1c186 --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2UnsupportedOptionException.cs @@ -0,0 +1,12 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; + +namespace PackageUploader.Application.Tools; + +/// +/// Thrown when the operation configuration contains an option that MakePkg.exe has no equivalent for. +/// The CLI fails fast rather than silently dropping the option. +/// +internal sealed class Msixvc2UnsupportedOptionException(string message) : Exception(message); diff --git a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs new file mode 100644 index 00000000..830c3b88 --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs @@ -0,0 +1,204 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#nullable enable + +using Microsoft.Extensions.Logging; +using PackageUploader.Application.Config; +using PackageUploader.ClientApi; +using System; +using System.IO; +using System.Text; + +namespace PackageUploader.Application.Tools; + +/// +/// Translates an into a MakePkg.exe "upload" command line. +/// +/// Every flag emitted here is grounded in existing repository code that already shells out to the tool. +/// The closest precedent is PackageUploadViewModel.BuildMsixvc2UploadArguments(), which handles the +/// same scenario as the CLI: an already-built .msixvc package on disk. That builder emits +/// upload /pd "<dir>" [/branch|/flight] [/market] [/storeid] /auth CacheableBrowser and deliberately +/// does NOT pass /msixvc2 — that flag only appears in Msixvc2UploadViewModel.BuildUploadArguments(), +/// which packs from a loose content folder via /d. The CLI mirrors the /pd form. +/// +/// Options that MakePkg.exe has no equivalent for are either warned about and ignored (when ignoring them +/// cannot change the outcome) or cause a (when it could). +/// +internal static class Msixvc2UploadArgumentBuilder +{ + /// + /// The only /auth value with precedent in this repository. Both UI argument builders emit this literal. + /// MakePkg.exe performs the interactive sign-in itself, so no other credential can be forwarded. + /// + private const string MakePkgAuthenticationValue = "CacheableBrowser"; + + public static string Build( + UploadXvcPackageOperationConfig config, + Msixvc2CommandLineContext commandLineContext, + string bigId, + bool supportsUploadSource, + ILogger logger) + { + ArgumentNullException.ThrowIfNull(config); + ArgumentNullException.ThrowIfNull(commandLineContext); + ArgumentNullException.ThrowIfNull(logger); + ArgumentException.ThrowIfNullOrWhiteSpace(bigId); + + var packageDirectory = GetPackageDirectory(config.PackageFilePath); + + ValidateUnsupportedOptions(config, commandLineContext, packageDirectory, logger); + + var args = new StringBuilder(); + args.Append("upload"); + args.Append(Invariant($" /pd \"{packageDirectory}\"")); + + if (!string.IsNullOrWhiteSpace(config.BranchFriendlyName)) + { + args.Append(Invariant($" /branch \"{config.BranchFriendlyName}\"")); + } + else if (!string.IsNullOrWhiteSpace(config.FlightName)) + { + args.Append(Invariant($" /flight \"{config.FlightName}\"")); + } + + if (!string.IsNullOrWhiteSpace(config.MarketGroupName)) + { + args.Append(Invariant($" /market \"{config.MarketGroupName}\"")); + } + + args.Append(Invariant($" /storeid \"{bigId}\"")); + + if (supportsUploadSource) + { + args.Append(Invariant($" /uploadsource {IngestionExtensions.PackageUploaderUploadSource}")); + } + + args.Append(Invariant($" /auth {MakePkgAuthenticationValue}")); + + return args.ToString(); + } + + private static void ValidateUnsupportedOptions( + UploadXvcPackageOperationConfig config, + Msixvc2CommandLineContext commandLineContext, + string packageDirectory, + ILogger logger) + { + ValidateGameAssets(config, packageDirectory, logger); + + // MakePkg.exe owns the upload lifecycle and reports completion itself, so a caller-supplied + // processing timeout cannot change the outcome. An explicitly configured 30 is indistinguishable + // from the default, so this is always a warning rather than an error. + logger.LogWarning( + "'minutesToWaitForProcessing' is not used for MSIXVC2 uploads; MakePkg.exe manages upload processing itself."); + + if (config.DeltaUpload) + { + logger.LogWarning( + "'deltaUpload' is not used for MSIXVC2 uploads and will be ignored; MakePkg.exe decides its own chunk reuse strategy."); + } + + // Availability/pre-download dates are applied today by a post-upload SetXvcConfigurationAsync call, + // which needs the specific GamePackage that was just uploaded. MakePkg.exe does not report that + // identity back, and guessing which package it created could reconfigure the wrong package. + if (config.AvailabilityDate?.IsEnabled == true) + { + throw new Msixvc2UnsupportedOptionException( + "'availabilityDate' cannot be applied during an MSIXVC2 upload because MakePkg.exe does not report which package it uploaded. " + + "Remove it from this config and set the availability date in Partner Center, or with a separate PackageUploader invocation, after the upload completes."); + } + + if (config.PreDownloadDate?.IsEnabled == true) + { + throw new Msixvc2UnsupportedOptionException( + "'preDownloadDate' cannot be applied during an MSIXVC2 upload because MakePkg.exe does not report which package it uploaded. " + + "Remove it from this config and set the pre-download date in Partner Center, or with a separate PackageUploader invocation, after the upload completes."); + } + + if (!string.IsNullOrWhiteSpace(commandLineContext.TenantId)) + { + throw new Msixvc2UnsupportedOptionException( + "--TenantId has no MakePkg.exe equivalent for MSIXVC2 uploads. Remove it and sign in with the account's default tenant."); + } + + // MakePkg.exe performs the sign-in itself and cannot accept a forwarded client secret or certificate. + // CacheableBrowser is the only /auth value with precedent in this repository. + if (commandLineContext.AuthenticationMethod is not IngestionExtensions.AuthenticationMethod.CacheableBrowser) + { + throw new Msixvc2UnsupportedOptionException( + $"--Authentication {commandLineContext.AuthenticationMethod} cannot be forwarded to MakePkg.exe for MSIXVC2 uploads, " + + "because MakePkg.exe performs its own interactive sign-in. " + + $"Use --Authentication {IngestionExtensions.AuthenticationMethod.CacheableBrowser}."); + } + } + + /// + /// MSIXVC2 packages do not use EKB or submission validator assets, and MakePkg.exe has no flags for them. + /// MakePkg.exe expects any such files to sit alongside the package, so assets already in the package + /// directory are harmless and are only warned about. Assets pointing elsewhere are a hard error, because + /// silently dropping a file the user deliberately placed somewhere else would change the outcome. + /// + private static void ValidateGameAssets(UploadXvcPackageOperationConfig config, string packageDirectory, ILogger logger) + { + if (config.GameAssets is null) + { + return; + } + + RequireInPackageDirectory(GameAssetPaths.EkbFilePath, config.GameAssets.EkbFilePath, packageDirectory); + RequireInPackageDirectory(GameAssetPaths.SubValFilePath, config.GameAssets.SubValFilePath, packageDirectory); + RequireInPackageDirectory(GameAssetPaths.SymbolsFilePath, config.GameAssets.SymbolsFilePath, packageDirectory); + RequireInPackageDirectory(GameAssetPaths.DiscLayoutFilePath, config.GameAssets.DiscLayoutFilePath, packageDirectory); + RequireInPackageDirectory(GameAssetPaths.SodbFilePath, config.GameAssets.SodbFilePath, packageDirectory); + + logger.LogWarning( + "'gameAssets' is not used for MSIXVC2 uploads and will be ignored. The configured assets already sit in the package directory '{PackageDirectory}', where MakePkg.exe expects them.", + packageDirectory); + } + + private static void RequireInPackageDirectory(string propertyName, string? assetPath, string packageDirectory) + { + if (string.IsNullOrWhiteSpace(assetPath)) + { + return; + } + + var assetDirectory = Path.GetDirectoryName(Path.GetFullPath(assetPath)); + + if (!string.Equals(assetDirectory, packageDirectory, StringComparison.OrdinalIgnoreCase)) + { + throw new Msixvc2UnsupportedOptionException( + $"'gameAssets.{ToCamelCase(propertyName)}' points at '{assetPath}', which is outside the package directory '{packageDirectory}'. " + + "MSIXVC2 uploads ignore gameAssets, and MakePkg.exe only picks up files that sit alongside the package, so this file would not be uploaded. " + + "Move it into the package directory or remove it from the config file."); + } + } + + private static string GetPackageDirectory(string packageFilePath) + { + // MakePkg.exe uploads an already-built package directory via /pd, not an individual package file. + var directory = Path.GetDirectoryName(Path.GetFullPath(packageFilePath)); + if (string.IsNullOrWhiteSpace(directory)) + { + throw new Msixvc2UnsupportedOptionException( + $"Could not determine the package directory for 'packageFilePath' value '{packageFilePath}'."); + } + + return directory; + } + + private static string ToCamelCase(string value) => char.ToLowerInvariant(value[0]) + value[1..]; + + private static string Invariant(FormattableString formattable) => FormattableString.Invariant(formattable); + + /// Names of the GameAssets properties, used for nameof() in error messages. + private static class GameAssetPaths + { + public const string EkbFilePath = nameof(EkbFilePath); + public const string SubValFilePath = nameof(SubValFilePath); + public const string SymbolsFilePath = nameof(SymbolsFilePath); + public const string DiscLayoutFilePath = nameof(DiscLayoutFilePath); + public const string SodbFilePath = nameof(SodbFilePath); + } +} diff --git a/src/PackageUploader.ClientApi.Test/Packaging/PackageFormatDetectorTest.cs b/src/PackageUploader.ClientApi.Test/Packaging/PackageFormatDetectorTest.cs new file mode 100644 index 00000000..ff71785a --- /dev/null +++ b/src/PackageUploader.ClientApi.Test/Packaging/PackageFormatDetectorTest.cs @@ -0,0 +1,114 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +#nullable enable + +using PackageUploader.ClientApi.Packaging; + +namespace PackageUploader.ClientApi.Test.Packaging; + +[TestClass] +public class PackageFormatDetectorTest +{ + private const int MinimumDetectableSize = 4096; + + private static string CreatePackage(string extension, byte[]? header = null, byte[]? footer = null) + { + var path = Path.Combine(Path.GetTempPath(), $"pu-detector-{Guid.NewGuid():N}{extension}"); + var contents = new byte[MinimumDetectableSize * 2]; + + if (header is not null) + { + Array.Copy(header, contents, header.Length); + } + + if (footer is not null) + { + Array.Copy(footer, 0, contents, contents.Length - footer.Length, footer.Length); + } + + File.WriteAllBytes(path, contents); + return path; + } + + [TestMethod] + public void IsLikelyMsixvc2Package_ZipLocalFileHeader_ReturnsTrue() + { + var path = CreatePackage(".msixvc", header: [0x50, 0x4B, 0x03, 0x04]); + try + { + Assert.IsTrue(PackageFormatDetector.IsLikelyMsixvc2Package(path)); + } + finally + { + File.Delete(path); + } + } + + [TestMethod] + public void IsLikelyMsixvc2Package_ZipEndOfCentralDirectorySignature_ReturnsTrue() + { + var path = CreatePackage(".msixvc", header: [0x01, 0x02, 0x03, 0x04], footer: [0x50, 0x4B, 0x05, 0x06]); + try + { + Assert.IsTrue(PackageFormatDetector.IsLikelyMsixvc2Package(path)); + } + finally + { + File.Delete(path); + } + } + + [TestMethod] + public void IsLikelyMsixvc2Package_NoZipSignatures_ReturnsFalse() + { + var path = CreatePackage(".msixvc", header: [0x01, 0x02, 0x03, 0x04]); + try + { + Assert.IsFalse(PackageFormatDetector.IsLikelyMsixvc2Package(path)); + } + finally + { + File.Delete(path); + } + } + + [TestMethod] + public void IsLikelyMsixvc2Package_WrongExtension_ReturnsFalse() + { + var path = CreatePackage(".xvc", header: [0x50, 0x4B, 0x03, 0x04]); + try + { + Assert.IsFalse(PackageFormatDetector.IsLikelyMsixvc2Package(path)); + } + finally + { + File.Delete(path); + } + } + + [TestMethod] + public void IsLikelyMsixvc2Package_FileTooSmall_ReturnsFalse() + { + var path = Path.Combine(Path.GetTempPath(), $"pu-detector-{Guid.NewGuid():N}.msixvc"); + File.WriteAllBytes(path, [0x50, 0x4B, 0x03, 0x04]); + try + { + Assert.IsFalse(PackageFormatDetector.IsLikelyMsixvc2Package(path)); + } + finally + { + File.Delete(path); + } + } + + [TestMethod] + public void IsLikelyMsixvc2Package_MissingFile_ReturnsFalse() => + Assert.IsFalse(PackageFormatDetector.IsLikelyMsixvc2Package(@"C:\does\not\exist.msixvc")); + + [TestMethod] + [DataRow("")] + [DataRow(" ")] + public void IsLikelyMsixvc2Package_EmptyPath_ReturnsFalse(string path) => + Assert.IsFalse(PackageFormatDetector.IsLikelyMsixvc2Package(path)); +} diff --git a/src/PackageUploader.ClientApi/PackageUploaderExtensions.cs b/src/PackageUploader.ClientApi/PackageUploaderExtensions.cs index 70572f4a..0189902f 100644 --- a/src/PackageUploader.ClientApi/PackageUploaderExtensions.cs +++ b/src/PackageUploader.ClientApi/PackageUploaderExtensions.cs @@ -14,6 +14,9 @@ public static class IngestionExtensions /// UploadSource value for the Xbox Game Package Manager (XGPM) UI. public const string XgpmUploadSource = UploadSourceConfig.XgpmSource; + /// UploadSource value for the Package Uploader CLI. + public const string PackageUploaderUploadSource = UploadSourceConfig.PackageUploaderSource; + public enum AuthenticationMethod { AppSecret, diff --git a/src/PackageUploader.ClientApi/Packaging/PackageFormatDetector.cs b/src/PackageUploader.ClientApi/Packaging/PackageFormatDetector.cs new file mode 100644 index 00000000..297bd8a0 --- /dev/null +++ b/src/PackageUploader.ClientApi/Packaging/PackageFormatDetector.cs @@ -0,0 +1,95 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.IO; + +namespace PackageUploader.ClientApi.Packaging; + +/// +/// Detects the on-disk format of game packages. Lives in PackageUploader.ClientApi so that both +/// PackageUploader.Application (CLI) and PackageUploader.UI share a single source of truth. +/// +public static class PackageFormatDetector +{ + private static readonly byte[] ZipLocalFileSignature = [0x50, 0x4B, 0x03, 0x04]; + private static readonly byte[] ZipEocdSignature = [0x50, 0x4B, 0x05, 0x06]; + private const int FirstReadSize = 4096; + private const int LastReadSize = 65 * 1024; + private const int MinFileNameOffset = 30; + + /// + /// Detects whether a .msixvc file is in MSIXVC2 format by checking for ZIP signatures. + /// MSIXVC2 packages are ZIP-based and contain standard ZIP headers, while MSIXVC1 + /// packages use a proprietary binary format without ZIP signatures. + /// + public static bool IsLikelyMsixvc2Package(string packagePath) + { + try + { + if (string.IsNullOrWhiteSpace(packagePath)) + return false; + + if (!packagePath.EndsWith(".msixvc", StringComparison.OrdinalIgnoreCase)) + return false; + + var fileInfo = new FileInfo(packagePath); + if (!fileInfo.Exists || fileInfo.Length < FirstReadSize) + return false; + + using var stream = File.OpenRead(packagePath); + + // Check first bytes for ZIP local file header at offset 0 + var firstBuffer = new byte[Math.Min(FirstReadSize, fileInfo.Length)]; + stream.ReadExactly(firstBuffer, 0, firstBuffer.Length); + + if (firstBuffer.Length >= MinFileNameOffset && + firstBuffer[0] == ZipLocalFileSignature[0] && + firstBuffer[1] == ZipLocalFileSignature[1] && + firstBuffer[2] == ZipLocalFileSignature[2] && + firstBuffer[3] == ZipLocalFileSignature[3]) + { + return true; + } + + // Check last 65KB for ZIP End of Central Directory signature + long lastChunkStart = Math.Max(0, fileInfo.Length - LastReadSize); + int lastChunkSize = (int)(fileInfo.Length - lastChunkStart); + var lastBuffer = new byte[lastChunkSize]; + stream.Position = lastChunkStart; + stream.ReadExactly(lastBuffer, 0, lastChunkSize); + + if (LastIndexOfSignature(lastBuffer, ZipEocdSignature) >= 0) + return true; + + return false; + } + catch (Exception) + { + return false; + } + } + + private static int LastIndexOfSignature(byte[] buffer, byte[] signature) + { + if (buffer.Length < signature.Length) + return -1; + + for (int i = buffer.Length - signature.Length; i >= 0; i--) + { + bool match = true; + for (int j = 0; j < signature.Length; j++) + { + if (buffer[i + j] != signature[j]) + { + match = false; + break; + } + } + if (match) + return i; + } + + return -1; + } +} diff --git a/src/PackageUploader.UI/Model/Xvc/XvcFile.cs b/src/PackageUploader.UI/Model/Xvc/XvcFile.cs index 39f9882e..7e27ef3f 100644 --- a/src/PackageUploader.UI/Model/Xvc/XvcFile.cs +++ b/src/PackageUploader.UI/Model/Xvc/XvcFile.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using PackageUploader.ClientApi.Packaging; using System; using System.Drawing; using System.IO; @@ -60,83 +61,12 @@ private static UInt32 NumberOfHashPagesForLevel(UInt64 dataPages, Int32 level) return (UInt32)((dataPages + divisor - 1) / divisor); } - private static readonly byte[] ZipLocalFileSignature = [0x50, 0x4B, 0x03, 0x04]; - private static readonly byte[] ZipEocdSignature = [0x50, 0x4B, 0x05, 0x06]; - private const int FirstReadSize = 4096; - private const int LastReadSize = 65 * 1024; - private const int MinFileNameOffset = 30; - /// /// Detects whether a .msixvc file is in MSIXVC2 format by checking for ZIP signatures. - /// MSIXVC2 packages are ZIP-based and contain standard ZIP headers, while MSIXVC1 - /// packages use a proprietary binary format without ZIP signatures. + /// The implementation lives in PackageUploader.ClientApi so the CLI and the UI share one source of truth. /// - public static bool IsLikelyMsixvc2Package(string packagePath) - { - try - { - if (!packagePath.EndsWith(".msixvc", StringComparison.OrdinalIgnoreCase)) - return false; - - var fileInfo = new FileInfo(packagePath); - if (!fileInfo.Exists || fileInfo.Length < FirstReadSize) - return false; - - using var stream = File.OpenRead(packagePath); - - // Check first bytes for ZIP local file header at offset 0 - var firstBuffer = new byte[Math.Min(FirstReadSize, fileInfo.Length)]; - stream.ReadExactly(firstBuffer, 0, firstBuffer.Length); - - if (firstBuffer.Length >= MinFileNameOffset && - firstBuffer[0] == ZipLocalFileSignature[0] && - firstBuffer[1] == ZipLocalFileSignature[1] && - firstBuffer[2] == ZipLocalFileSignature[2] && - firstBuffer[3] == ZipLocalFileSignature[3]) - { - return true; - } - - // Check last 65KB for ZIP End of Central Directory signature - long lastChunkStart = Math.Max(0, fileInfo.Length - LastReadSize); - int lastChunkSize = (int)(fileInfo.Length - lastChunkStart); - var lastBuffer = new byte[lastChunkSize]; - stream.Position = lastChunkStart; - stream.ReadExactly(lastBuffer, 0, lastChunkSize); - - if (LastIndexOfSignature(lastBuffer, ZipEocdSignature) >= 0) - return true; - - return false; - } - catch (Exception) - { - return false; - } - } - - private static int LastIndexOfSignature(byte[] buffer, byte[] signature) - { - if (buffer.Length < signature.Length) - return -1; - - for (int i = buffer.Length - signature.Length; i >= 0; i--) - { - bool match = true; - for (int j = 0; j < signature.Length; j++) - { - if (buffer[i + j] != signature[j]) - { - match = false; - break; - } - } - if (match) - return i; - } - - return -1; - } + public static bool IsLikelyMsixvc2Package(string packagePath) => + PackageFormatDetector.IsLikelyMsixvc2Package(packagePath); public static void GetBuildAndKeyId(string packagePath, out Guid buildId, out Guid keyId) { From a40a1d8967ac80133f0849114f37355a44351fc8 Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" Date: Wed, 12 Aug 2026 09:55:56 -0700 Subject: [PATCH 02/10] Add environment-variable loop-breaker against MakePkg.exe recursion PackageFormatDetector.IsLikelyMsixvc2Package is a heuristic: its fallback check scans the trailing bytes of the package for the 4-byte ZIP EOCD signature, which an encrypted XVC1 tail can contain by chance. Because MakePkg.exe shells back out to PackageUploader.exe for XVC1 uploads, a false positive there is unbounded rather than merely wrong. Stamp PACKAGEUPLOADER_MSIXVC2_DELEGATED=1 onto every MakePkg.exe child process, and refuse to delegate when that variable is already present in our own environment. Any MakePkg.exe that shells back to us inherits the stamp, so the cycle breaks after exactly one hop no matter what the format heuristic decides. Format detection remains the primary guard. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Tools/Msixvc2DelegationGuardTest.cs | 39 ++++++++++++++++ .../Tools/Msixvc2DelegationGuard.cs | 44 +++++++++++++++++++ .../Tools/Msixvc2ProcessRunner.cs | 7 +++ 3 files changed, 90 insertions(+) create mode 100644 src/PackageUploader.Application.Test/Tools/Msixvc2DelegationGuardTest.cs create mode 100644 src/PackageUploader.Application/Tools/Msixvc2DelegationGuard.cs diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2DelegationGuardTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2DelegationGuardTest.cs new file mode 100644 index 00000000..5ffd7d7d --- /dev/null +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2DelegationGuardTest.cs @@ -0,0 +1,39 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using PackageUploader.Application.Tools; +using System; + +namespace PackageUploader.Application.Test.Tools; + +[TestClass] +public class Msixvc2DelegationGuardTest +{ + private string? _originalValue; + + [TestInitialize] + public void Initialize() => + _originalValue = Environment.GetEnvironmentVariable(Msixvc2DelegationGuard.EnvironmentVariableName); + + [TestCleanup] + public void Cleanup() => + Environment.SetEnvironmentVariable(Msixvc2DelegationGuard.EnvironmentVariableName, _originalValue); + + [TestMethod] + public void IsDelegatedInvocation_WhenVariableAbsent_IsFalse() + { + Environment.SetEnvironmentVariable(Msixvc2DelegationGuard.EnvironmentVariableName, null); + + Assert.IsFalse(new Msixvc2DelegationGuard().IsDelegatedInvocation); + } + + [TestMethod] + public void IsDelegatedInvocation_WhenVariableSet_IsTrue() + { + Environment.SetEnvironmentVariable( + Msixvc2DelegationGuard.EnvironmentVariableName, + Msixvc2DelegationGuard.EnvironmentVariableValue); + + Assert.IsTrue(new Msixvc2DelegationGuard().IsDelegatedInvocation); + } +} diff --git a/src/PackageUploader.Application/Tools/Msixvc2DelegationGuard.cs b/src/PackageUploader.Application/Tools/Msixvc2DelegationGuard.cs new file mode 100644 index 00000000..3b101746 --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2DelegationGuard.cs @@ -0,0 +1,44 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; + +namespace PackageUploader.Application.Tools; + +/// +/// Second, independent barrier against PackageUploader.exe ↔ MakePkg.exe process recursion. +/// +/// The primary guard is package-format detection, but PackageFormatDetector.IsLikelyMsixvc2Package +/// is a heuristic: its fallback check scans the trailing bytes of the file for the 4-byte ZIP +/// end-of-central-directory signature, which an encrypted XVC1 tail can contain by chance. A false positive +/// there would be unbounded, because MakePkg.exe shells back out to PackageUploader.exe for XVC1 uploads +/// (the legacy makepkg.exe help text literally describes its upload verb as operating "via the PackageUploader tool"): +/// +/// PackageUploader.exe → MakePkg.exe → PackageUploader.exe → MakePkg.exe → ... +/// +/// So PackageUploader stamps an environment variable onto every MakePkg.exe child process it starts, and +/// refuses to delegate again if it sees that variable already set in its own environment. Any MakePkg.exe +/// that shells back to us inherits the stamp, which breaks the cycle after exactly one hop regardless of +/// what the format heuristic decides. +/// +internal interface IMsixvc2DelegationGuard +{ + /// + /// True when this PackageUploader process was itself started (directly or indirectly) by a MakePkg.exe + /// that we delegated to, meaning delegating again would risk an unbounded process cycle. + /// + bool IsDelegatedInvocation { get; } +} + +/// +internal sealed class Msixvc2DelegationGuard : IMsixvc2DelegationGuard +{ + /// Environment variable stamped onto the MakePkg.exe child process. + public const string EnvironmentVariableName = "PACKAGEUPLOADER_MSIXVC2_DELEGATED"; + + /// Value stamped onto the MakePkg.exe child process. + public const string EnvironmentVariableValue = "1"; + + public bool IsDelegatedInvocation => + !string.IsNullOrEmpty(Environment.GetEnvironmentVariable(EnvironmentVariableName)); +} diff --git a/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs b/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs index 1c1b0048..ec000a3f 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs @@ -34,6 +34,13 @@ public async Task RunAsync(string executablePath, string arguments, Cancell EnableRaisingEvents = true, }; + // Recursion breaker: MakePkg.exe shells back out to PackageUploader.exe for XVC1 uploads. Stamping + // the child environment means any PackageUploader.exe started beneath us can see that it is already + // a delegated invocation and refuse to delegate again, bounding the cycle at a single hop even if + // the MSIXVC2 format heuristic false-positives. See Msixvc2DelegationGuard. + process.StartInfo.Environment[Msixvc2DelegationGuard.EnvironmentVariableName] = + Msixvc2DelegationGuard.EnvironmentVariableValue; + process.OutputDataReceived += (_, e) => { if (!string.IsNullOrEmpty(e.Data)) From c9040c8e25a1e8cd928e877d62ad8377c09f1385 Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" Date: Wed, 12 Aug 2026 09:56:08 -0700 Subject: [PATCH 03/10] Forward non-interactive credentials to MakePkg.exe for MSIXVC2 uploads The builder previously rejected every --Authentication value except CacheableBrowser, which made MSIXVC2 upload impossible from any non-interactive pipeline - PackageUploader.exe's primary use case. Grounded against the verbatim help output of the MSIXVC2-capable packaging tool (makepkg2.exe upload /?, version 2604.405.14000.0): /auth accepts Default, Browser, CacheableBrowser, AzureCli, ManagedIdentity, ManagedIdentityFederated, Environment, AzurePipelines, ClientSecret and ClientCertificate, alongside /tenantid, /clientid, /clientsecret, /certthumbprint, /certstore, /certlocation and /resourceid. So forward the configured identity instead of rejecting it. AppSecret and AppCert map onto ClientSecret and ClientCertificate, which are the same AAD application flows under the tool's names; the other ten map verbatim. The /tenantid hard-fail is likewise removed, since the flag exists. Two configurations are still rejected, because the tool genuinely has no equivalent: a certificate FILE path (it selects certificates from a Windows store by thumbprint only) and a certificate SUBJECT. Also drop /uploadsource entirely. The flag exists but its enum accepts only 'makepkg2' and 'XGPM' - there is no value representing PackageUploader, so the previously emitted value was invalid. The tool's own default is used. Redact /clientsecret and /certpassword from the logged argument string, and document in the README that a secret passed this way is visible in the process table, recommending the credential-free methods on shared agents. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 28 +- .../Tools/LoggerMockExtensions.cs | 3 + .../Tools/Msixvc2UploadArgumentBuilderTest.cs | 326 +++++++++++++----- .../Tools/TempPackageFile.cs | 3 + .../UploadXvcPackageOperationMsixvc2Test.cs | 65 +++- .../Extensions/HostExtensions.cs | 44 ++- .../Operations/UploadXvcPackageOperation.cs | 38 +- .../Tools/IMsixvc2UploadToolProvider.cs | 3 - .../Tools/Msixvc2CapabilityPlaceholder.cs | 59 +--- .../Tools/Msixvc2CommandLineContext.cs | 33 +- .../Tools/Msixvc2UploadArgumentBuilder.cs | 179 ++++++++-- .../PackageUploaderExtensions.cs | 3 - 12 files changed, 579 insertions(+), 205 deletions(-) diff --git a/README.md b/README.md index 5beb5987..03953fd0 100644 --- a/README.md +++ b/README.md @@ -200,11 +200,37 @@ The following configuration options behave differently on the MSIXVC2 path: | `deltaUpload` | Ignored with a warning. | | `availabilityDate` / `preDownloadDate` | Not supported. MakePkg.exe does not report back the identity of the package it uploaded, so the post-upload XVC configuration step cannot target it. Set these dates with a separate `PublishPackages` or configuration invocation. | | `productId` | Supported. It is resolved to the corresponding Big ID, which is what MakePkg.exe requires. | -| Authentication | MakePkg.exe performs its own interactive, cached sign-in. Only `CacheableBrowser` is supported; other `--Authentication` values and `--TenantId` fail with an explicit error rather than being silently ignored. | +| Authentication | Supported, including non-interactive/CI authentication. See below. | If no MSIXVC2-capable `MakePkg.exe` is available, the operation fails with an actionable error instead of attempting an upload that cannot succeed. +#### Authentication on the MSIXVC2 path + +MakePkg.exe acquires its own token, so PackageUploader forwards the identity you configured rather than forcing an +interactive sign-in. `--Authentication`, `--TenantId`, and the client id / secret / certificate values from your +configuration file are passed through, so an unattended pipeline using a service principal keeps working. + +| `--Authentication` | Forwarded to MakePkg.exe as | +| --- | --- | +| `AppSecret` | `ClientSecret` (PackageUploader's legacy name for the same AAD application secret flow) | +| `AppCert` | `ClientCertificate` | +| `Default`, `Browser`, `CacheableBrowser`, `AzureCli`, `ManagedIdentity`, `ManagedIdentityFederated`, `Environment`, `AzurePipelines`, `ClientSecret`, `ClientCertificate` | The same value | + +Two limitations, both because MakePkg.exe has no corresponding option: + +- **Certificates must live in a Windows certificate store.** MakePkg.exe selects a certificate by thumbprint + (`/certthumbprint`, `/certstore`, `/certlocation`) and has no option naming a certificate file, so a PFX path + (`ClientCertificateAuthInfo:CertificatePath`) is rejected with an explicit error. Import the certificate into a store + and use `AppCert` with `AadAuthInfo:CertificateThumbprint`. +- **Certificates cannot be selected by subject.** `AadAuthInfo:CertificateSubject` is rejected; use + `AadAuthInfo:CertificateThumbprint`. + +> **Note:** with `AppSecret`/`ClientSecret`, the secret is passed to MakePkg.exe on its command line and is therefore +> visible to anything that can read the process table on the machine. PackageUploader redacts it from its own logs. On +> shared build agents prefer `Environment`, `AzurePipelines`, `ManagedIdentity`, or `ManagedIdentityFederated`, none of +> which put a credential on the command line. + ### Available parameters | Parameter | Description | diff --git a/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs b/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs index 8412cc9a..38cb0e24 100644 --- a/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs +++ b/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs @@ -14,6 +14,9 @@ public static void VerifyLogErrorContains(this Mock> loggerMock, s public static void VerifyLogWarningContains(this Mock loggerMock, string expectedSubstring) => VerifyLogContains(loggerMock, LogLevel.Warning, expectedSubstring); + public static void VerifyLogWarningContains(this Mock> loggerMock, string expectedSubstring) => + VerifyLogContains(loggerMock, LogLevel.Warning, expectedSubstring); + private static void VerifyLogContains(Mock loggerMock, LogLevel level, string expectedSubstring) where TLogger : class, ILogger => loggerMock.Verify( diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs index 49c94ce5..34353c17 100644 --- a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs @@ -1,13 +1,18 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +#nullable enable + using Microsoft.Extensions.Logging; +using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; using PackageUploader.Application.Config; -using PackageUploader.Application.Test.Config; using PackageUploader.Application.Tools; using PackageUploader.ClientApi; +using PackageUploader.ClientApi.Client.Ingestion.Models; using PackageUploader.ClientApi.Models; +using System; +using System.IO; namespace PackageUploader.Application.Test.Tools; @@ -15,180 +20,311 @@ namespace PackageUploader.Application.Test.Tools; public class Msixvc2UploadArgumentBuilderTest { private const string BigId = "9NBLGGH4R315"; - private static readonly string PackageFilePath = Path.Combine(Path.GetTempPath(), "packages", "game.msixvc"); - private static readonly string PackageDirectory = Path.GetDirectoryName(Path.GetFullPath(PackageFilePath))!; - private static readonly Msixvc2CommandLineContext CacheableBrowserContext = - new(IngestionExtensions.AuthenticationMethod.CacheableBrowser, TenantId: null); + private Mock _loggerMock = null!; - private readonly Mock _loggerMock = new(); + [TestInitialize] + public void Initialize() + { + _loggerMock = new Mock(); + } - private static UploadXvcPackageOperationConfig CreateConfig() => new TestUploadXvcPackageOperationConfig + private static UploadXvcPackageOperationConfig CreateConfig(string packagePath) => new() { - OperationName = "UploadXvcPackage", BigId = BigId, BranchFriendlyName = "Main", MarketGroupName = "default", - PackageFilePath = PackageFilePath, + PackageFilePath = packagePath, }; - private string Build(UploadXvcPackageOperationConfig config, Msixvc2CommandLineContext? context = null, bool supportsUploadSource = false) => - Msixvc2UploadArgumentBuilder.Build(config, context ?? CacheableBrowserContext, BigId, supportsUploadSource, _loggerMock.Object); + private static Msixvc2CommandLineContext BrowserContext() => + new(IngestionExtensions.AuthenticationMethod.CacheableBrowser); + + private string Build(UploadXvcPackageOperationConfig config, Msixvc2CommandLineContext context) => + Msixvc2UploadArgumentBuilder.Build(config, context, BigId, _loggerMock.Object); [TestMethod] - public void Build_BranchConfig_ProducesExpectedArguments() + public void Build_WithBranch_ProducesExpectedArguments() { - var args = Build(CreateConfig()); + using var package = TempPackageFile.CreateMsixvc2(); + var config = CreateConfig(package.Path); + + var arguments = Build(config, BrowserContext()); Assert.AreEqual( - $"upload /pd \"{PackageDirectory}\" /branch \"Main\" /market \"default\" /storeid \"{BigId}\" /auth CacheableBrowser", - args); + $"upload /pd \"{package.Directory}\" /branch \"Main\" /market \"default\" /storeid \"{BigId}\" /auth CacheableBrowser", + arguments); } [TestMethod] - public void Build_FlightConfigWithUploadSource_ProducesExpectedArguments() + public void Build_WithFlight_UsesFlightInsteadOfBranch() { - var config = CreateConfig(); + using var package = TempPackageFile.CreateMsixvc2(); + var config = CreateConfig(package.Path); config.BranchFriendlyName = null; - config.FlightName = "PreviewFlight"; - config.MarketGroupName = "NorthAmerica"; + config.FlightName = "Alpha Flight"; + + var arguments = Build(config, BrowserContext()); + + StringAssert.Contains(arguments, "/flight \"Alpha Flight\""); + Assert.IsFalse(arguments.Contains("/branch", StringComparison.Ordinal)); + } + + [TestMethod] + public void Build_DoesNotEmitUploadSource() + { + // makepkg2's /uploadsource enum only accepts 'makepkg2' and 'XGPM'; there is no value that + // represents PackageUploader, so the flag is deliberately omitted. + using var package = TempPackageFile.CreateMsixvc2(); + + var arguments = Build(CreateConfig(package.Path), BrowserContext()); + + Assert.IsFalse(arguments.Contains("/uploadsource", StringComparison.OrdinalIgnoreCase)); + } + + [TestMethod] + public void Build_WithoutMarketGroup_OmitsMarketFlag() + { + using var package = TempPackageFile.CreateMsixvc2(); + var config = CreateConfig(package.Path); + config.MarketGroupName = null; + + var arguments = Build(config, BrowserContext()); + + Assert.IsFalse(arguments.Contains("/market", StringComparison.Ordinal)); + } + + #region Authentication + + [TestMethod] + public void Build_WithAppSecret_MapsToClientSecretAndForwardsCredentials() + { + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.AppSecret, + TenantId: "tenant-1", + ClientId: "client-1", + ClientSecret: "secret-1"); - var args = Build(config, supportsUploadSource: true); + var arguments = Build(CreateConfig(package.Path), context); Assert.AreEqual( - $"upload /pd \"{PackageDirectory}\" /flight \"PreviewFlight\" /market \"NorthAmerica\" /storeid \"{BigId}\" /uploadsource PackageUploader /auth CacheableBrowser", - args); + $"upload /pd \"{package.Directory}\" /branch \"Main\" /market \"default\" /storeid \"{BigId}\" " + + "/auth ClientSecret /tenantid \"tenant-1\" /clientid \"client-1\" /clientsecret \"secret-1\"", + arguments); } - /// - /// /msixvc2 belongs to the pack-from-content-folder form (Msixvc2UploadViewModel, which uses /d). - /// The already-built-package form used by the CLI mirrors PackageUploadViewModel and must not emit it. - /// [TestMethod] - public void Build_DoesNotEmitMsixvc2FlagForAlreadyBuiltPackage() => - Assert.DoesNotContain("/msixvc2", Build(CreateConfig())); + public void Build_WithClientSecret_ForwardsVerbatim() + { + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.ClientSecret, + TenantId: "tenant-1", + ClientId: "client-1", + ClientSecret: "secret-1"); + + var arguments = Build(CreateConfig(package.Path), context); + + StringAssert.Contains(arguments, "/auth ClientSecret"); + } [TestMethod] - public void Build_ResolvedBigIdIsUsedInsteadOfConfigProductId() + public void Build_WithAppCert_MapsToClientCertificateAndForwardsStoreDetails() { - var config = CreateConfig(); - config.BigId = null; - config.ProductId = "00000000-0000-0000-0000-000000000001"; + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.AppCert, + TenantId: "tenant-1", + ClientId: "client-1", + CertificateThumbprint: "ABC123", + CertificateStore: "My", + CertificateLocation: "CurrentUser"); - var args = Msixvc2UploadArgumentBuilder.Build(config, CacheableBrowserContext, "9RESOLVED123", supportsUploadSource: false, _loggerMock.Object); + var arguments = Build(CreateConfig(package.Path), context); - StringAssert.Contains(args, "/storeid \"9RESOLVED123\""); + Assert.AreEqual( + $"upload /pd \"{package.Directory}\" /branch \"Main\" /market \"default\" /storeid \"{BigId}\" " + + "/auth ClientCertificate /tenantid \"tenant-1\" /clientid \"client-1\" " + + "/certthumbprint \"ABC123\" /certstore \"My\" /certlocation \"CurrentUser\"", + arguments); } [TestMethod] - public void Build_GameAssetsInPackageDirectory_WarnsAndContinues() + public void Build_WithAzurePipelines_ForwardsMethodWithoutCredentials() { - var config = CreateConfig(); - config.GameAssets = new GameAssets - { - EkbFilePath = Path.Combine(PackageDirectory, "game.ekb"), - SubValFilePath = Path.Combine(PackageDirectory, "validator.xml"), - }; + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext(IngestionExtensions.AuthenticationMethod.AzurePipelines); - var args = Build(config); + var arguments = Build(CreateConfig(package.Path), context); - StringAssert.Contains(args, "/storeid"); - _loggerMock.VerifyLogWarningContains("'gameAssets' is not used for MSIXVC2 uploads"); + StringAssert.Contains(arguments, "/auth AzurePipelines"); + Assert.IsFalse(arguments.Contains("/clientsecret", StringComparison.Ordinal)); } [TestMethod] - public void Build_GameAssetsOutsidePackageDirectory_Throws() + public void Build_WithManagedIdentityFederated_ForwardsResourceId() { - var strayPath = Path.Combine(Path.GetTempPath(), "elsewhere", "game.ekb"); - var config = CreateConfig(); - config.GameAssets = new GameAssets - { - EkbFilePath = strayPath, - SubValFilePath = Path.Combine(PackageDirectory, "validator.xml"), - }; + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.ManagedIdentityFederated, + ClientId: "client-1", + ResourceId: "resource-1"); - var ex = Assert.ThrowsExactly(() => Build(config)); + var arguments = Build(CreateConfig(package.Path), context); - StringAssert.Contains(ex.Message, "gameAssets.ekbFilePath"); - StringAssert.Contains(ex.Message, strayPath); - StringAssert.Contains(ex.Message, PackageDirectory); + StringAssert.Contains(arguments, "/auth ManagedIdentityFederated"); + StringAssert.Contains(arguments, "/resourceid \"resource-1\""); } [TestMethod] - public void Build_MinutesToWaitForProcessing_WarnsAndContinues() + public void Build_WithClientSecretButNoSecret_Throws() { - var config = CreateConfig(); - config.MinutesToWaitForProcessing = 60; + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.AppSecret, + TenantId: "tenant-1", + ClientId: "client-1"); - var args = Build(config); + var exception = Assert.ThrowsExactly( + () => Build(CreateConfig(package.Path), context)); - StringAssert.Contains(args, "/storeid"); - _loggerMock.VerifyLogWarningContains("'minutesToWaitForProcessing' is not used for MSIXVC2 uploads"); + StringAssert.Contains(exception.Message, "/clientsecret"); } [TestMethod] - public void Build_DeltaUpload_WarnsAndContinues() + public void Build_WithCertificateFilePath_Throws() { - var config = CreateConfig(); - config.DeltaUpload = true; + // makepkg2 authenticates from a certificate store only; there is no flag naming a PFX file. + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.ClientCertificate, + TenantId: "tenant-1", + ClientId: "client-1", + CertificatePath: @"C:\certs\app.pfx"); + + var exception = Assert.ThrowsExactly( + () => Build(CreateConfig(package.Path), context)); + + StringAssert.Contains(exception.Message, "app.pfx"); + } + + [TestMethod] + public void Build_WithCertificateSubject_Throws() + { + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.AppCert, + TenantId: "tenant-1", + ClientId: "client-1", + CertificateSubject: "CN=Contoso"); - var args = Build(config); + var exception = Assert.ThrowsExactly( + () => Build(CreateConfig(package.Path), context)); - StringAssert.Contains(args, "/storeid"); - _loggerMock.VerifyLogWarningContains("'deltaUpload' is not used for MSIXVC2 uploads"); + StringAssert.Contains(exception.Message, "CN=Contoso"); } [TestMethod] - public void Build_AvailabilityDate_Throws() + public void Build_WithCertificateButNoThumbprint_Throws() { - var config = CreateConfig(); - config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(1) }; + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.AppCert, + TenantId: "tenant-1", + ClientId: "client-1"); - var ex = Assert.ThrowsExactly(() => Build(config)); + var exception = Assert.ThrowsExactly( + () => Build(CreateConfig(package.Path), context)); - StringAssert.Contains(ex.Message, "'availabilityDate' cannot be applied"); + StringAssert.Contains(exception.Message, "/certthumbprint"); } + #endregion + + #region Unsupported options + [TestMethod] - public void Build_DisabledAvailabilityDate_DoesNotThrow() + public void Build_WithAvailabilityDate_Throws() { - var config = CreateConfig(); - config.AvailabilityDate = new GamePackageDate { IsEnabled = false }; + using var package = TempPackageFile.CreateMsixvc2(); + var config = CreateConfig(package.Path); + config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow }; - StringAssert.Contains(Build(config), "/storeid"); + var exception = Assert.ThrowsExactly( + () => Build(config, BrowserContext())); + + StringAssert.Contains(exception.Message, "availabilityDate"); } [TestMethod] - public void Build_PreDownloadDate_Throws() + public void Build_WithPreDownloadDate_Throws() { - var config = CreateConfig(); - config.PreDownloadDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(1) }; + using var package = TempPackageFile.CreateMsixvc2(); + var config = CreateConfig(package.Path); + config.PreDownloadDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow }; - var ex = Assert.ThrowsExactly(() => Build(config)); + var exception = Assert.ThrowsExactly( + () => Build(config, BrowserContext())); - StringAssert.Contains(ex.Message, "'preDownloadDate' cannot be applied"); + StringAssert.Contains(exception.Message, "preDownloadDate"); } [TestMethod] - public void Build_TenantId_Throws() + public void Build_WithDeltaUpload_WarnsAndContinues() { - var ex = Assert.ThrowsExactly(() => Build( - CreateConfig(), - new Msixvc2CommandLineContext(IngestionExtensions.AuthenticationMethod.CacheableBrowser, "contoso.onmicrosoft.com"))); + using var package = TempPackageFile.CreateMsixvc2(); + var config = CreateConfig(package.Path); + config.DeltaUpload = true; - StringAssert.Contains(ex.Message, "TenantId"); + var arguments = Build(config, BrowserContext()); + + Assert.IsFalse(string.IsNullOrEmpty(arguments)); + _loggerMock.VerifyLogWarningContains("deltaUpload"); } [TestMethod] - [DataRow(IngestionExtensions.AuthenticationMethod.AppSecret)] - [DataRow(IngestionExtensions.AuthenticationMethod.Browser)] - [DataRow(IngestionExtensions.AuthenticationMethod.AzureCli)] - public void Build_NonCacheableBrowserAuthentication_Throws(IngestionExtensions.AuthenticationMethod method) + public void Build_AlwaysWarnsAboutMinutesToWaitForProcessing() { - var ex = Assert.ThrowsExactly(() => Build( - CreateConfig(), - new Msixvc2CommandLineContext(method, TenantId: null))); + using var package = TempPackageFile.CreateMsixvc2(); + + Build(CreateConfig(package.Path), BrowserContext()); - StringAssert.Contains(ex.Message, method.ToString()); + _loggerMock.VerifyLogWarningContains("minutesToWaitForProcessing"); } + + [TestMethod] + public void Build_WithGameAssetsInPackageDirectory_WarnsAndContinues() + { + using var package = TempPackageFile.CreateMsixvc2(); + var config = CreateConfig(package.Path); + config.GameAssets = new GameAssets + { + EkbFilePath = Path.Combine(package.Directory, "package.ekb"), + SubValFilePath = Path.Combine(package.Directory, "validator.xml"), + }; + + var arguments = Build(config, BrowserContext()); + + Assert.IsFalse(string.IsNullOrEmpty(arguments)); + _loggerMock.VerifyLogWarningContains("gameAssets"); + } + + [TestMethod] + public void Build_WithGameAssetsOutsidePackageDirectory_Throws() + { + using var package = TempPackageFile.CreateMsixvc2(); + var config = CreateConfig(package.Path); + var strayPath = Path.Combine(Path.GetTempPath(), "elsewhere", "package.ekb"); + config.GameAssets = new GameAssets { EkbFilePath = strayPath }; + + var exception = Assert.ThrowsExactly( + () => Build(config, BrowserContext())); + + StringAssert.Contains(exception.Message, "ekbFilePath"); + StringAssert.Contains(exception.Message, package.Directory); + } + + #endregion } + diff --git a/src/PackageUploader.Application.Test/Tools/TempPackageFile.cs b/src/PackageUploader.Application.Test/Tools/TempPackageFile.cs index 59dea67d..eac03499 100644 --- a/src/PackageUploader.Application.Test/Tools/TempPackageFile.cs +++ b/src/PackageUploader.Application.Test/Tools/TempPackageFile.cs @@ -14,6 +14,9 @@ internal sealed class TempPackageFile : IDisposable public string Path { get; } + /// Directory containing the package, which is what MakePkg.exe's /pd flag takes. + public string Directory => System.IO.Path.GetDirectoryName(System.IO.Path.GetFullPath(Path))!; + private TempPackageFile(string path) => Path = path; /// Creates a .msixvc file that starts with the ZIP local file header, i.e. an MSIXVC2 package. diff --git a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs index 063993f3..734e7cde 100644 --- a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs +++ b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs @@ -25,6 +25,7 @@ public class UploadXvcPackageOperationMsixvc2Test private readonly Mock> _loggerMock = new(); private readonly Mock _toolProviderMock = new(); private readonly Mock _processRunnerMock = new(); + private readonly Mock _delegationGuardMock = new(); private UploadXvcPackageOperation CreateOperation( UploadXvcPackageOperationConfig config, @@ -34,7 +35,8 @@ private UploadXvcPackageOperation CreateOperation( Options.Create(config), _toolProviderMock.Object, _processRunnerMock.Object, - new Msixvc2CommandLineContext(authenticationMethod, TenantId: null)); + _delegationGuardMock.Object, + new Msixvc2CommandLineContext(authenticationMethod)); private static UploadXvcPackageOperationConfig CreateConfig(string packageFilePath) => new TestUploadXvcPackageOperationConfig { @@ -56,11 +58,10 @@ private static GameProduct CreateProduct(string productId, string bigId) return product; } - private void SetUpAvailableTool(bool supportsUploadSource = false) + private void SetUpAvailableTool() { _toolProviderMock.SetupGet(x => x.IsAvailable).Returns(true); _toolProviderMock.SetupGet(x => x.ExecutablePath).Returns(ResolvedMakePkgPath); - _toolProviderMock.SetupGet(x => x.SupportsUploadSource).Returns(supportsUploadSource); } private void SetUpSuccessfulRun() => @@ -265,6 +266,64 @@ public async Task NonZeroExitCode_FailsOperation() _loggerMock.VerifyLogErrorContains("MakePkg.exe failed with exit code 7"); } + /// + /// Loop-breaker, direction 1: a normal (non-delegated) invocation delegates as usual. + /// + [TestMethod] + public async Task DelegationGuardAbsent_Delegates() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + _delegationGuardMock.SetupGet(x => x.IsDelegatedInvocation).Returns(false); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _processRunnerMock.Verify( + x => x.RunAsync(ResolvedMakePkgPath, ExpectedArguments(package.Path), It.IsAny()), + Times.Once); + } + + /// + /// Loop-breaker, direction 2: when this process was itself launched by MakePkg.exe, never delegate back. + /// Format detection is a heuristic, so a false positive on an XVC1 package could otherwise produce + /// PackageUploader.exe -> MakePkg.exe -> PackageUploader.exe recursion without bound. + /// + [TestMethod] + public async Task DelegationGuardPresent_NeverShellsOutAndTakesLegacyPath() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + _delegationGuardMock.SetupGet(x => x.IsDelegatedInvocation).Returns(true); + + _serviceMock + .Setup(x => x.GetProductByBigIdAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new InvalidOperationException("took the normal XVC upload path")); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _processRunnerMock.VerifyNoOtherCalls(); + _serviceMock.Verify(x => x.GetProductByBigIdAsync(BigId, It.IsAny()), Times.Once); + } + + [TestMethod] + public async Task DelegationGuardPresent_LogsWarning() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + _delegationGuardMock.SetupGet(x => x.IsDelegatedInvocation).Returns(true); + _serviceMock + .Setup(x => x.GetProductByBigIdAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new InvalidOperationException("took the normal XVC upload path")); + + await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + _loggerMock.VerifyLogWarningContains(Msixvc2DelegationGuard.EnvironmentVariableName); + } + [TestMethod] public async Task Cancellation_PropagatesTokenAndFailsOperation() { diff --git a/src/PackageUploader.Application/Extensions/HostExtensions.cs b/src/PackageUploader.Application/Extensions/HostExtensions.cs index c1694fd1..0073c47c 100644 --- a/src/PackageUploader.Application/Extensions/HostExtensions.cs +++ b/src/PackageUploader.Application/Extensions/HostExtensions.cs @@ -74,10 +74,9 @@ internal static HostApplicationBuilder ConfigureServices(this HostApplicationBui hostAppBuilder.Services.AddSingleton(new DataOutputOptions(isData)); hostAppBuilder.Services.AddPackageUploaderService(parseResult.GetValue(CommandLineHelper.AuthenticationMethodOption)); - hostAppBuilder.Services.AddSingleton(new Msixvc2CommandLineContext( - parseResult.GetValue(CommandLineHelper.AuthenticationMethodOption), - parseResult.GetValue(CommandLineHelper.TenantIdOption))); + hostAppBuilder.Services.AddSingleton(BuildMsixvc2CommandLineContext(hostAppBuilder, parseResult)); hostAppBuilder.Services.AddSingleton(); + hostAppBuilder.Services.AddSingleton(); // TODO(GDK-release): swap Msixvc2CapabilityPlaceholder for an adapter over // PackageUploader.ClientApi.Tools.IMsixvc2ToolResolver once PR # merges. hostAppBuilder.Services.AddSingleton(); @@ -120,6 +119,45 @@ internal static HostApplicationBuilder ConfigureServices(this HostApplicationBui return hostAppBuilder; } + /// + /// Collects the authentication surface MakePkg.exe needs for MSIXVC2 uploads. PackageUploader spreads + /// this across a command line option (--Authentication) and several configuration sections, and + /// MakePkg.exe accepts the same credential material through /auth, /tenantid, /clientid, + /// /clientsecret, /certthumbprint, /certstore, /certlocation and /resourceid — so a CI pipeline using + /// a service principal keeps working when the upload is delegated. + /// + private static Msixvc2CommandLineContext BuildMsixvc2CommandLineContext(HostApplicationBuilder hostAppBuilder, ParseResult parseResult) + { + var configuration = hostAppBuilder.Configuration; + var aad = configuration.GetSection(AadAuthInfo.ConfigName); + var clientSecret = configuration.GetSection(ClientSecretAuthInfo.ConfigName); + var clientCertificate = configuration.GetSection(ClientCertificateAuthInfo.ConfigName); + + static string First(params string[] values) => + values.FirstOrDefault(v => !string.IsNullOrWhiteSpace(v)); + + return new Msixvc2CommandLineContext( + parseResult.GetValue(CommandLineHelper.AuthenticationMethodOption), + TenantId: First( + parseResult.GetValue(CommandLineHelper.TenantIdOption), + aad[nameof(AadAuthInfo.TenantId)], + clientSecret[nameof(ClientSecretAuthInfo.TenantId)], + clientCertificate[nameof(ClientCertificateAuthInfo.TenantId)]), + ClientId: First( + aad[nameof(AadAuthInfo.ClientId)], + clientSecret[nameof(ClientSecretAuthInfo.ClientId)], + clientCertificate[nameof(ClientCertificateAuthInfo.ClientId)]), + ClientSecret: First( + aad[nameof(AzureApplicationSecretAuthInfo.ClientSecret)], + clientSecret[nameof(ClientSecretAuthInfo.ClientSecret)]), + CertificateThumbprint: aad[nameof(AzureApplicationCertificateAuthInfo.CertificateThumbprint)], + CertificateSubject: aad[nameof(AzureApplicationCertificateAuthInfo.CertificateSubject)], + CertificateStore: aad[nameof(AzureApplicationCertificateAuthInfo.CertificateStore)], + CertificateLocation: aad[nameof(AzureApplicationCertificateAuthInfo.CertificateLocation)], + CertificatePath: clientCertificate[nameof(ClientCertificateAuthInfo.CertificatePath)], + ResourceId: aad["ResourceId"]); + } + internal static HostApplicationBuilder ConfigureAppConfiguration(this HostApplicationBuilder hostAppBuilder, ParseResult parseResult) { var configFile = parseResult.GetValue(CommandLineHelper.ConfigFileOption); diff --git a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs index 34bd4ba4..12adb787 100644 --- a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs +++ b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs @@ -9,6 +9,7 @@ using PackageUploader.ClientApi; using PackageUploader.ClientApi.Packaging; using System; +using System.Text.RegularExpressions; using System.Threading; using System.Threading.Tasks; @@ -20,6 +21,7 @@ internal class UploadXvcPackageOperation( IOptions config, IMsixvc2UploadToolProvider msixvc2ToolProvider, IMsixvc2ProcessRunner msixvc2ProcessRunner, + IMsixvc2DelegationGuard msixvc2DelegationGuard, Msixvc2CommandLineContext msixvc2CommandLineContext) : Operation(logger) { private readonly IPackageUploaderService _storeBrokerService = storeBrokerService ?? throw new ArgumentNullException(nameof(storeBrokerService)); @@ -27,6 +29,7 @@ internal class UploadXvcPackageOperation( private readonly UploadXvcPackageOperationConfig _config = config?.Value ?? throw new ArgumentNullException(nameof(config)); private readonly IMsixvc2UploadToolProvider _msixvc2ToolProvider = msixvc2ToolProvider ?? throw new ArgumentNullException(nameof(msixvc2ToolProvider)); private readonly IMsixvc2ProcessRunner _msixvc2ProcessRunner = msixvc2ProcessRunner ?? throw new ArgumentNullException(nameof(msixvc2ProcessRunner)); + private readonly IMsixvc2DelegationGuard _msixvc2DelegationGuard = msixvc2DelegationGuard ?? throw new ArgumentNullException(nameof(msixvc2DelegationGuard)); private readonly Msixvc2CommandLineContext _msixvc2CommandLineContext = msixvc2CommandLineContext ?? throw new ArgumentNullException(nameof(msixvc2CommandLineContext)); protected override async Task ProcessAsync(CancellationToken ct) @@ -39,8 +42,23 @@ protected override async Task ProcessAsync(CancellationToken ct) // package-format detection itself (not a config flag) so it cannot be bypassed by configuration. if (PackageFormatDetector.IsLikelyMsixvc2Package(_config.PackageFilePath)) { - await UploadMsixvc2PackageAsync(ct).ConfigureAwait(false); - return; + // SAFETY (defense in depth): format detection is a heuristic and can false-positive on an XVC1 + // package whose encrypted tail happens to contain the ZIP end-of-central-directory signature. + // If this process was itself started by a MakePkg.exe we delegated to, delegating again would be + // exactly the unbounded cycle above, so fall through to the normal upload path instead. + if (_msixvc2DelegationGuard.IsDelegatedInvocation) + { + _logger.LogWarning( + "Package '{PackageFilePath}' looks like MSIXVC2, but this PackageUploader process was started by MakePkg.exe ({EnvironmentVariable} is set). " + + "Uploading directly instead of delegating back to MakePkg.exe, to avoid an infinite MakePkg.exe/PackageUploader.exe loop.", + _config.PackageFilePath, + Msixvc2DelegationGuard.EnvironmentVariableName); + } + else + { + await UploadMsixvc2PackageAsync(ct).ConfigureAwait(false); + return; + } } var product = await _storeBrokerService.GetProductAsync(_config, ct).ConfigureAwait(false); @@ -76,9 +94,11 @@ private async Task UploadMsixvc2PackageAsync(CancellationToken ct) var bigId = await ResolveBigIdAsync(ct).ConfigureAwait(false); var executablePath = _msixvc2ToolProvider.ExecutablePath; - var arguments = Msixvc2UploadArgumentBuilder.Build(_config, _msixvc2CommandLineContext, bigId, _msixvc2ToolProvider.SupportsUploadSource, _logger); + var arguments = Msixvc2UploadArgumentBuilder.Build(_config, _msixvc2CommandLineContext, bigId, _logger); - _logger.LogInformation("Running {executablePath} {arguments}", executablePath, arguments); + // The argument string can carry a client secret, so log a redacted form. The child process still + // receives the real value. + _logger.LogInformation("Running {executablePath} {arguments}", executablePath, Redact(arguments)); var exitCode = await _msixvc2ProcessRunner.RunAsync(executablePath, arguments, ct).ConfigureAwait(false); @@ -90,6 +110,16 @@ private async Task UploadMsixvc2PackageAsync(CancellationToken ct) _logger.LogInformation("MSIXVC2 package uploaded successfully."); } + /// + /// Replaces the value of any secret-bearing MakePkg.exe flag so credentials never reach the log file. + /// + private static string Redact(string arguments) => + Regex.Replace( + arguments, + "(?/(?:clientsecret|certpassword))\\s+\"[^\"]*\"", + "${flag} \"***\"", + RegexOptions.IgnoreCase); + /// /// MakePkg.exe identifies the product by Store ID (/storeid) only. When the config supplies a ProductId /// instead, resolve it to the corresponding Big ID through the ingestion service rather than failing. diff --git a/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs b/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs index c83d27e3..2ffd77d0 100644 --- a/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs +++ b/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs @@ -17,7 +17,4 @@ internal interface IMsixvc2UploadToolProvider /// Full path to the resolved MakePkg.exe (or makepkg2.exe fallback). Null when unavailable. string? ExecutablePath { get; } - - /// True when the resolved tool understands the /uploadsource flag. - bool SupportsUploadSource { get; } } diff --git a/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs b/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs index 78e240b5..bb512890 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs @@ -3,10 +3,6 @@ #nullable enable -using Microsoft.Extensions.Logging; -using System; -using System.Diagnostics; - namespace PackageUploader.Application.Tools; /// @@ -20,66 +16,13 @@ namespace PackageUploader.Application.Tools; /// Until then this placeholder reports the capability as always available and relies on /// MakePkg.exe being resolvable from PATH. /// -internal sealed class Msixvc2CapabilityPlaceholder(ILogger logger) : IMsixvc2UploadToolProvider +internal sealed class Msixvc2CapabilityPlaceholder : IMsixvc2UploadToolProvider { private const string PlaceholderExecutable = "MakePkg.exe"; - private const int UploadSourceProbeTimeoutMs = 5000; - - private readonly ILogger _logger = logger ?? throw new ArgumentNullException(nameof(logger)); - private bool? _supportsUploadSource; // TODO(GDK-release): replace with IMsixvc2ToolResolver.IsMsixvc2Supported(). public bool IsAvailable => true; // TODO(GDK-release): replace with IMsixvc2ToolResolver.Resolve()?.ExecutablePath. public string? ExecutablePath => PlaceholderExecutable; - - /// - /// Probes the tool for /uploadsource support, mirroring the UI's behaviour - /// (see Msixvc2UploadViewModel.SupportsUploadSourceFlag). Result is cached per process. - /// - public bool SupportsUploadSource => _supportsUploadSource ??= ProbeUploadSourceSupport(); - - private bool ProbeUploadSourceSupport() - { - var executablePath = ExecutablePath; - if (string.IsNullOrEmpty(executablePath)) - { - return false; - } - - try - { - using var process = new Process - { - StartInfo = new ProcessStartInfo - { - FileName = executablePath, - Arguments = "supports uploadsource", - UseShellExecute = false, - RedirectStandardOutput = false, - RedirectStandardError = false, - CreateNoWindow = true, - }, - }; - - process.Start(); - if (process.WaitForExit(UploadSourceProbeTimeoutMs)) - { - var supported = process.ExitCode == 0; - _logger.LogDebug("MakePkg /uploadsource probe: {Result} (exit code {ExitCode}).", - supported ? "supported" : "not supported", process.ExitCode); - return supported; - } - - try { process.Kill(entireProcessTree: true); } catch { /* best effort */ } - _logger.LogWarning("MakePkg /uploadsource probe timed out after {TimeoutMs}ms.", UploadSourceProbeTimeoutMs); - } - catch (Exception ex) - { - _logger.LogWarning(ex, "Failed to probe MakePkg for /uploadsource support."); - } - - return false; - } } diff --git a/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs b/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs index f6444146..410e1c29 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs @@ -8,7 +8,34 @@ namespace PackageUploader.Application.Tools; /// -/// Command line values that are not part of the bound operation configuration but are still needed -/// to build MakePkg.exe arguments. +/// Command line and configuration values that are not part of the bound operation configuration but are +/// still needed to build MakePkg.exe arguments — principally the authentication surface, which +/// PackageUploader resolves from a mixture of command line options and configuration sections. /// -internal sealed record Msixvc2CommandLineContext(IngestionExtensions.AuthenticationMethod AuthenticationMethod, string? TenantId); +/// The --Authentication value the user selected. +/// AAD tenant, forwarded to MakePkg.exe via /tenantid. +/// AAD application (client) id, forwarded via /clientid. +/// AAD application secret, forwarded via /clientsecret. +/// Certificate thumbprint, forwarded via /certthumbprint. +/// +/// Certificate subject name. MakePkg.exe has no equivalent flag, so this is only carried so the builder +/// can fail with an actionable message instead of silently authenticating as the wrong identity. +/// +/// Certificate store name, forwarded via /certstore. +/// Certificate store location, forwarded via /certlocation. +/// +/// Path to a PFX/PKCS12 certificate file. MakePkg.exe exposes /certpassword but no flag naming the +/// certificate file itself, so this is only carried so the builder can fail with an actionable message. +/// +/// Azure resource id, forwarded via /resourceid for ManagedIdentityFederated. +internal sealed record Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod AuthenticationMethod, + string? TenantId = null, + string? ClientId = null, + string? ClientSecret = null, + string? CertificateThumbprint = null, + string? CertificateSubject = null, + string? CertificateStore = null, + string? CertificateLocation = null, + string? CertificatePath = null, + string? ResourceId = null); diff --git a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs index 830c3b88..a1586366 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs @@ -7,6 +7,7 @@ using PackageUploader.Application.Config; using PackageUploader.ClientApi; using System; +using System.Collections.Generic; using System.IO; using System.Text; @@ -15,12 +16,25 @@ namespace PackageUploader.Application.Tools; /// /// Translates an into a MakePkg.exe "upload" command line. /// -/// Every flag emitted here is grounded in existing repository code that already shells out to the tool. -/// The closest precedent is PackageUploadViewModel.BuildMsixvc2UploadArguments(), which handles the -/// same scenario as the CLI: an already-built .msixvc package on disk. That builder emits -/// upload /pd "<dir>" [/branch|/flight] [/market] [/storeid] /auth CacheableBrowser and deliberately -/// does NOT pass /msixvc2 — that flag only appears in Msixvc2UploadViewModel.BuildUploadArguments(), -/// which packs from a loose content folder via /d. The CLI mirrors the /pd form. +/// Every flag emitted here is grounded in the verbatim help output of the MSIXVC2-capable packaging tool +/// (makepkg2.exe upload /?, version 2604.405.14000.0), cross-checked against the two existing UI +/// argument builders. The closest in-repo precedent is PackageUploadViewModel.BuildMsixvc2UploadArguments(), +/// which handles the same scenario as the CLI: an already-built .msixvc package on disk. That builder emits +/// the /pd form and deliberately does NOT pass /msixvc2 — that flag only appears in +/// Msixvc2UploadViewModel.BuildUploadArguments(), which packs from a loose content folder via /d. +/// +/// Notable grounding results: +/// +/// /auth accepts Default, Browser, CacheableBrowser, AzureCli, ManagedIdentity, +/// ManagedIdentityFederated, Environment, AzurePipelines, ClientSecret and ClientCertificate — so +/// non-interactive CI authentication is fully supported and is forwarded rather than rejected. +/// /tenantid, /clientid, /clientsecret, /certthumbprint, /certstore, +/// /certlocation and /resourceid all exist and carry the credential material. +/// There is no flag naming a certificate file, and no flag for a certificate subject, +/// so those two configurations are rejected rather than silently authenticating as a different identity. +/// /uploadsource exists but its enum only accepts makepkg2 and XGPM. There is no +/// value representing PackageUploader, so the flag is omitted and the tool's own default is used. +/// /// /// Options that MakePkg.exe has no equivalent for are either warned about and ignored (when ignoring them /// cannot change the outcome) or cause a (when it could). @@ -28,16 +42,27 @@ namespace PackageUploader.Application.Tools; internal static class Msixvc2UploadArgumentBuilder { /// - /// The only /auth value with precedent in this repository. Both UI argument builders emit this literal. - /// MakePkg.exe performs the interactive sign-in itself, so no other credential can be forwarded. + /// The /auth values accepted by the MSIXVC2 packaging tool, taken verbatim from its help output. + /// PackageUploader's own AuthenticationMethod enum uses the same names for all of these. /// - private const string MakePkgAuthenticationValue = "CacheableBrowser"; + private static readonly HashSet DirectlySupportedAuthenticationMethods = + [ + IngestionExtensions.AuthenticationMethod.Default, + IngestionExtensions.AuthenticationMethod.Browser, + IngestionExtensions.AuthenticationMethod.CacheableBrowser, + IngestionExtensions.AuthenticationMethod.AzureCli, + IngestionExtensions.AuthenticationMethod.ManagedIdentity, + IngestionExtensions.AuthenticationMethod.ManagedIdentityFederated, + IngestionExtensions.AuthenticationMethod.Environment, + IngestionExtensions.AuthenticationMethod.AzurePipelines, + IngestionExtensions.AuthenticationMethod.ClientSecret, + IngestionExtensions.AuthenticationMethod.ClientCertificate, + ]; public static string Build( UploadXvcPackageOperationConfig config, Msixvc2CommandLineContext commandLineContext, string bigId, - bool supportsUploadSource, ILogger logger) { ArgumentNullException.ThrowIfNull(config); @@ -47,7 +72,7 @@ public static string Build( var packageDirectory = GetPackageDirectory(config.PackageFilePath); - ValidateUnsupportedOptions(config, commandLineContext, packageDirectory, logger); + ValidateUnsupportedOptions(config, packageDirectory, logger); var args = new StringBuilder(); args.Append("upload"); @@ -69,19 +94,125 @@ public static string Build( args.Append(Invariant($" /storeid \"{bigId}\"")); - if (supportsUploadSource) + AppendAuthenticationArguments(args, commandLineContext); + + return args.ToString(); + } + + /// + /// Forwards the selected authentication method and its credential material. MakePkg.exe performs the + /// token acquisition itself, so PackageUploader hands over the same identity the user configured rather + /// than forcing an interactive sign-in — otherwise MSIXVC2 upload would be impossible from any + /// non-interactive pipeline. + /// + private static void AppendAuthenticationArguments(StringBuilder args, Msixvc2CommandLineContext context) + { + var method = ResolveAuthenticationMethod(context.AuthenticationMethod); + + args.Append(Invariant($" /auth {method}")); + + if (!string.IsNullOrWhiteSpace(context.TenantId)) { - args.Append(Invariant($" /uploadsource {IngestionExtensions.PackageUploaderUploadSource}")); + args.Append(Invariant($" /tenantid \"{context.TenantId}\"")); } - args.Append(Invariant($" /auth {MakePkgAuthenticationValue}")); + if (!string.IsNullOrWhiteSpace(context.ClientId)) + { + args.Append(Invariant($" /clientid \"{context.ClientId}\"")); + } - return args.ToString(); + switch (method) + { + case IngestionExtensions.AuthenticationMethod.ClientSecret: + RequireCredential(context.ClientId, "/clientid", "a client id", "AadAuthInfo:ClientId or ClientSecretAuthInfo:ClientId"); + RequireCredential(context.ClientSecret, "/clientsecret", "a client secret", "AadAuthInfo:ClientSecret or ClientSecretAuthInfo:ClientSecret"); + args.Append(Invariant($" /clientsecret \"{context.ClientSecret}\"")); + break; + + case IngestionExtensions.AuthenticationMethod.ClientCertificate: + AppendCertificateArguments(args, context); + break; + + case IngestionExtensions.AuthenticationMethod.ManagedIdentityFederated: + if (!string.IsNullOrWhiteSpace(context.ResourceId)) + { + args.Append(Invariant($" /resourceid \"{context.ResourceId}\"")); + } + break; + } + } + + private static void AppendCertificateArguments(StringBuilder args, Msixvc2CommandLineContext context) + { + // makepkg2 authenticates from a certificate STORE (thumbprint + store + location). It exposes + // /certpassword but no flag naming a certificate file, so a PFX path cannot be forwarded. + if (!string.IsNullOrWhiteSpace(context.CertificatePath)) + { + throw new Msixvc2UnsupportedOptionException( + $"Certificate file authentication ('{context.CertificatePath}') cannot be forwarded to MakePkg.exe for MSIXVC2 uploads, " + + "because MakePkg.exe only accepts a certificate from a Windows certificate store (/certthumbprint, /certstore, /certlocation) " + + "and has no option naming a certificate file. " + + $"Import the certificate into a store and use --Authentication {IngestionExtensions.AuthenticationMethod.AppCert} " + + "with AadAuthInfo:CertificateThumbprint, or choose a different authentication method."); + } + + // makepkg2 has no certificate-subject option. Resolving the subject ourselves and forwarding the + // resulting thumbprint would be guesswork about which certificate the user meant. + if (!string.IsNullOrWhiteSpace(context.CertificateSubject)) + { + throw new Msixvc2UnsupportedOptionException( + $"Certificate subject authentication ('{context.CertificateSubject}') cannot be forwarded to MakePkg.exe for MSIXVC2 uploads, " + + "because MakePkg.exe selects certificates by thumbprint only. " + + "Set AadAuthInfo:CertificateThumbprint instead of AadAuthInfo:CertificateSubject."); + } + + RequireCredential(context.ClientId, "/clientid", "a client id", "AadAuthInfo:ClientId"); + RequireCredential(context.CertificateThumbprint, "/certthumbprint", "a certificate thumbprint", "AadAuthInfo:CertificateThumbprint"); + + args.Append(Invariant($" /certthumbprint \"{context.CertificateThumbprint}\"")); + + if (!string.IsNullOrWhiteSpace(context.CertificateStore)) + { + args.Append(Invariant($" /certstore \"{context.CertificateStore}\"")); + } + + if (!string.IsNullOrWhiteSpace(context.CertificateLocation)) + { + args.Append(Invariant($" /certlocation \"{context.CertificateLocation}\"")); + } + } + + /// + /// Maps PackageUploader's AuthenticationMethod onto a /auth value MakePkg.exe accepts. + /// All but two names are shared verbatim. AppSecret and AppCert are PackageUploader's legacy names for + /// the same AAD application flows that MakePkg.exe calls ClientSecret and ClientCertificate — both + /// authenticate an AAD application with, respectively, a client secret or a store certificate, so the + /// rename is a straight alias rather than a behavioral change. + /// + private static IngestionExtensions.AuthenticationMethod ResolveAuthenticationMethod( + IngestionExtensions.AuthenticationMethod method) => method switch + { + IngestionExtensions.AuthenticationMethod.AppSecret => IngestionExtensions.AuthenticationMethod.ClientSecret, + IngestionExtensions.AuthenticationMethod.AppCert => IngestionExtensions.AuthenticationMethod.ClientCertificate, + _ when DirectlySupportedAuthenticationMethods.Contains(method) => method, + _ => throw new Msixvc2UnsupportedOptionException( + $"--Authentication {method} has no MakePkg.exe equivalent for MSIXVC2 uploads. " + + "MakePkg.exe accepts: Default, Browser, CacheableBrowser, AzureCli, ManagedIdentity, " + + "ManagedIdentityFederated, Environment, AzurePipelines, ClientSecret, ClientCertificate."), + }; + + private static void RequireCredential(string? value, string flag, string description, string configPath) + { + if (string.IsNullOrWhiteSpace(value)) + { + throw new Msixvc2UnsupportedOptionException( + $"MakePkg.exe requires {description} ({flag}) for this authentication method during an MSIXVC2 upload, " + + $"but none was configured. Set {configPath} in the config file."); + } } private static void ValidateUnsupportedOptions( UploadXvcPackageOperationConfig config, - Msixvc2CommandLineContext commandLineContext, string packageDirectory, ILogger logger) { @@ -115,22 +246,6 @@ private static void ValidateUnsupportedOptions( "'preDownloadDate' cannot be applied during an MSIXVC2 upload because MakePkg.exe does not report which package it uploaded. " + "Remove it from this config and set the pre-download date in Partner Center, or with a separate PackageUploader invocation, after the upload completes."); } - - if (!string.IsNullOrWhiteSpace(commandLineContext.TenantId)) - { - throw new Msixvc2UnsupportedOptionException( - "--TenantId has no MakePkg.exe equivalent for MSIXVC2 uploads. Remove it and sign in with the account's default tenant."); - } - - // MakePkg.exe performs the sign-in itself and cannot accept a forwarded client secret or certificate. - // CacheableBrowser is the only /auth value with precedent in this repository. - if (commandLineContext.AuthenticationMethod is not IngestionExtensions.AuthenticationMethod.CacheableBrowser) - { - throw new Msixvc2UnsupportedOptionException( - $"--Authentication {commandLineContext.AuthenticationMethod} cannot be forwarded to MakePkg.exe for MSIXVC2 uploads, " + - "because MakePkg.exe performs its own interactive sign-in. " + - $"Use --Authentication {IngestionExtensions.AuthenticationMethod.CacheableBrowser}."); - } } /// diff --git a/src/PackageUploader.ClientApi/PackageUploaderExtensions.cs b/src/PackageUploader.ClientApi/PackageUploaderExtensions.cs index 0189902f..70572f4a 100644 --- a/src/PackageUploader.ClientApi/PackageUploaderExtensions.cs +++ b/src/PackageUploader.ClientApi/PackageUploaderExtensions.cs @@ -14,9 +14,6 @@ public static class IngestionExtensions /// UploadSource value for the Xbox Game Package Manager (XGPM) UI. public const string XgpmUploadSource = UploadSourceConfig.XgpmSource; - /// UploadSource value for the Package Uploader CLI. - public const string PackageUploaderUploadSource = UploadSourceConfig.PackageUploaderSource; - public enum AuthenticationMethod { AppSecret, From eb3067eb1f60df1a5dd286c99048bf9cabbf76e6 Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" Date: Wed, 12 Aug 2026 10:01:33 -0700 Subject: [PATCH 04/10] Promote MSIXVC2 credential-exposure guidance and pin the makepkg2 caveat Delegating the upload means a client secret has to be handed to MakePkg.exe on its command line, where it is visible in the process table for the lifetime of the child. MakePkg.exe offers no out-of-band credential path, so redacting our own logs does not address it. Raise the guidance from a trailing note to a prominent warning callout, name the credential-free methods to prefer on shared agents, and reference it from the config table. Also record in the argument builder's docs that the mapping was verified against makepkg2.exe rather than the renamed MakePkg.exe from the merged GDK, so whoever revisits this knows /auth is the first thing to re-check if the merged tool diverges. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 18 ++++++++++++------ .../Tools/Msixvc2UploadArgumentBuilder.cs | 6 ++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 03953fd0..6d28dc6f 100644 --- a/README.md +++ b/README.md @@ -200,7 +200,7 @@ The following configuration options behave differently on the MSIXVC2 path: | `deltaUpload` | Ignored with a warning. | | `availabilityDate` / `preDownloadDate` | Not supported. MakePkg.exe does not report back the identity of the package it uploaded, so the post-upload XVC configuration step cannot target it. Set these dates with a separate `PublishPackages` or configuration invocation. | | `productId` | Supported. It is resolved to the corresponding Big ID, which is what MakePkg.exe requires. | -| Authentication | Supported, including non-interactive/CI authentication. See below. | +| Authentication | Supported, including non-interactive/CI authentication — but see the credential-exposure warning in [Authentication on the MSIXVC2 path](#authentication-on-the-msixvc2-path). | If no MSIXVC2-capable `MakePkg.exe` is available, the operation fails with an actionable error instead of attempting an upload that cannot succeed. @@ -211,6 +211,17 @@ MakePkg.exe acquires its own token, so PackageUploader forwards the identity you interactive sign-in. `--Authentication`, `--TenantId`, and the client id / secret / certificate values from your configuration file are passed through, so an unattended pipeline using a service principal keeps working. +> [!WARNING] +> **Avoid secret-bearing authentication methods on shared machines.** Because the upload is delegated to a separate +> process, a client secret has to be handed to MakePkg.exe on its command line, where it is visible to anything that can +> read the process table for as long as the upload runs. MakePkg.exe offers no way to pass a credential out of band. +> PackageUploader redacts the secret from its own logs, but that does not protect the command line itself. +> +> On shared build agents, prefer an authentication method that puts no credential on the command line: +> **`Environment`**, **`AzurePipelines`**, **`ManagedIdentity`**, or **`ManagedIdentityFederated`**. These are forwarded +> as just `/auth `, and MakePkg.exe obtains the token from the ambient environment itself. Only use +> `AppSecret`/`ClientSecret` where you control the machine and trust every process on it. + | `--Authentication` | Forwarded to MakePkg.exe as | | --- | --- | | `AppSecret` | `ClientSecret` (PackageUploader's legacy name for the same AAD application secret flow) | @@ -226,11 +237,6 @@ Two limitations, both because MakePkg.exe has no corresponding option: - **Certificates cannot be selected by subject.** `AadAuthInfo:CertificateSubject` is rejected; use `AadAuthInfo:CertificateThumbprint`. -> **Note:** with `AppSecret`/`ClientSecret`, the secret is passed to MakePkg.exe on its command line and is therefore -> visible to anything that can read the process table on the machine. PackageUploader redacts it from its own logs. On -> shared build agents prefer `Environment`, `AzurePipelines`, `ManagedIdentity`, or `ManagedIdentityFederated`, none of -> which put a credential on the command line. - ### Available parameters | Parameter | Description | diff --git a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs index a1586366..12fd1d50 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs @@ -36,6 +36,12 @@ namespace PackageUploader.Application.Tools; /// value representing PackageUploader, so the flag is omitted and the tool's own default is used. /// /// +/// CAVEAT: the binary this mapping was verified against is makepkg2.exe, not the renamed +/// MakePkg.exe that ships with the GDK once the two tools are merged. The legacy makepkg.exe +/// is demonstrably a different surface (it has /tenantid but no /auth at all), so if the merged +/// MakePkg.exe diverges on /auth, this mapping — and especially +/// — is the first thing to re-verify against its help output. +/// /// Options that MakePkg.exe has no equivalent for are either warned about and ignored (when ignoring them /// cannot change the outcome) or cause a (when it could). /// From 1022382dba46e3e797ae04a095f33e81a4cd3536 Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" Date: Wed, 12 Aug 2026 10:21:03 -0700 Subject: [PATCH 05/10] Stop naming the internal-only makepkg2 NuGet package in CLI error text Microsoft.Xbox.Packaging.Tools.makepkg2 ships on an internal-only feed, so pointing an external customer at it tells them to install something they cannot obtain. The GDK is the complete answer on its own: an installed GDK ships both makepkg.exe and makepkg2.exe side by side in \bin, so no NuGet package is required. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Operations/UploadXvcPackageOperation.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs index 12adb787..7e918eda 100644 --- a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs +++ b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs @@ -88,7 +88,7 @@ private async Task UploadMsixvc2PackageAsync(CancellationToken ct) { throw new InvalidOperationException( "The package is an MSIXVC2 package, but no MSIXVC2-capable MakePkg.exe was found. " + - "Install the latest Microsoft GDK (or the Microsoft.Xbox.Packaging.Tools package) and try again."); + "Install the latest Microsoft GDK and try again."); } var bigId = await ResolveBigIdAsync(ct).ConfigureAwait(false); From b32e883d40630a1061344c80c3e25bc0a3905b0b Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" Date: Wed, 12 Aug 2026 10:57:23 -0700 Subject: [PATCH 06/10] Match repo convention by removing file-level nullable directives This branch introduced the repository's only six `#nullable enable` directives. The repo does use nullable reference types, but always project-wide via `enable` in the .csproj, never by file-level directive. Drop the directives so the branch matches. Five of the six were in projects where NRT is off, so their `?` annotations were load-bearing and are stripped alongside the directive. The sixth (Msixvc2UploadArgumentBuilderTest.cs) is in a project that already enables NRT project-wide, so its directive was a no-op and only the line is removed. Annotation-only: no null check, guard, or fail-fast is altered, and `?.`, `!`, and Nullable value types such as DateTime? are untouched. Because the compiler can no longer express it, the null contract that the annotations carried is now stated explicitly in XML docs, most importantly on IMsixvc2UploadToolProvider: ExecutablePath is null or empty whenever IsAvailable is false, neither member may throw when no tool is available, and both must report a single shared resolution. That contract is what the post-rebase adapter over IMsixvc2ToolResolver has to honor. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Tools/Msixvc2UploadArgumentBuilderTest.cs | 2 -- .../UploadXvcPackageOperationMsixvc2Test.cs | 5 +++- .../Tools/IMsixvc2UploadToolProvider.cs | 29 +++++++++++++++---- .../Tools/Msixvc2CapabilityPlaceholder.cs | 6 ++-- .../Tools/Msixvc2CommandLineContext.cs | 25 +++++++++------- .../Tools/Msixvc2UploadArgumentBuilder.cs | 14 ++++++--- .../Packaging/PackageFormatDetectorTest.cs | 4 +-- 7 files changed, 56 insertions(+), 29 deletions(-) diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs index 34353c17..0e3f4963 100644 --- a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs @@ -1,8 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#nullable enable - using Microsoft.Extensions.Logging; using Microsoft.VisualStudio.TestTools.UnitTesting; using Moq; diff --git a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs index 734e7cde..cfc426ec 100644 --- a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs +++ b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs @@ -134,7 +134,10 @@ public async Task Msixvc2PackageWithoutCapability_FailsAndDoesNotShellOut() { using var package = TempPackageFile.CreateMsixvc2(); _toolProviderMock.SetupGet(x => x.IsAvailable).Returns(false); - _toolProviderMock.SetupGet(x => x.ExecutablePath).Returns((string?)null); + // Exercises IMsixvc2UploadToolProvider's documented contract: ExecutablePath is null when + // IsAvailable is false. null! (not a behavior change - the value is still null) because this + // test project compiles with nullable reference types while the interface's project does not. + _toolProviderMock.SetupGet(x => x.ExecutablePath).Returns((string)null!); var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); diff --git a/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs b/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs index 2ffd77d0..1fccfa32 100644 --- a/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs +++ b/src/PackageUploader.Application/Tools/IMsixvc2UploadToolProvider.cs @@ -1,20 +1,39 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#nullable enable - namespace PackageUploader.Application.Tools; /// /// Narrow abstraction over "is an MSIXVC2-capable MakePkg.exe available, and where is it?". /// PackageUploader.Application depends only on this interface so that the underlying capability /// resolution can be swapped without touching any consuming code. +/// +/// CONTRACT — implementations must honor all of the following. These are stated here because this +/// project does not compile with nullable reference types, so the compiler cannot express them: +/// +/// is false exactly when no MSIXVC2-capable tool could be found. +/// is NULL OR EMPTY whenever is false, and +/// is a usable full path whenever it is true. Callers must tolerate a null path. +/// Neither member may THROW when no tool is available. "Unavailable" is an ordinary, expected +/// outcome that reports as a clean, actionable error; +/// an exception escaping either member would instead surface as an unhandled failure. +/// Both members should reflect a SINGLE underlying resolution. An implementation that resolves +/// separately per member can double any probing work and can disagree with itself between the two reads, +/// so resolve once and have both members report that one result. +/// /// internal interface IMsixvc2UploadToolProvider { - /// True when an MSIXVC2-capable packaging tool is installed and usable. + /// + /// True when an MSIXVC2-capable packaging tool is installed and usable. Never throws; + /// "not available" is reported as false rather than as an exception. + /// bool IsAvailable { get; } - /// Full path to the resolved MakePkg.exe (or makepkg2.exe fallback). Null when unavailable. - string? ExecutablePath { get; } + /// + /// Full path to the resolved MakePkg.exe (or makepkg2.exe fallback). + /// NULL OR EMPTY when is false — callers must check before use. + /// Never throws. + /// + string ExecutablePath { get; } } diff --git a/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs b/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs index bb512890..6c38f484 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs @@ -1,8 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#nullable enable - namespace PackageUploader.Application.Tools; /// @@ -24,5 +22,7 @@ internal sealed class Msixvc2CapabilityPlaceholder : IMsixvc2UploadToolProvider public bool IsAvailable => true; // TODO(GDK-release): replace with IMsixvc2ToolResolver.Resolve()?.ExecutablePath. - public string? ExecutablePath => PlaceholderExecutable; + // The replacement must return null/empty (never throw) when Resolve() finds no capable tool, + // and must share a single Resolve() call with IsAvailable. See IMsixvc2UploadToolProvider's contract. + public string ExecutablePath => PlaceholderExecutable; } diff --git a/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs b/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs index 410e1c29..024e318b 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2CommandLineContext.cs @@ -1,8 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#nullable enable - using PackageUploader.ClientApi; namespace PackageUploader.Application.Tools; @@ -11,6 +9,11 @@ namespace PackageUploader.Application.Tools; /// Command line and configuration values that are not part of the bound operation configuration but are /// still needed to build MakePkg.exe arguments — principally the authentication surface, which /// PackageUploader resolves from a mixture of command line options and configuration sections. +/// +/// Every parameter except is OPTIONAL and may be null or empty — +/// which credentials are present depends on the authentication method the user chose. The builder decides +/// which ones a given method actually requires and fails with an actionable message when one is missing; +/// consumers must not assume any of them is populated. /// /// The --Authentication value the user selected. /// AAD tenant, forwarded to MakePkg.exe via /tenantid. @@ -30,12 +33,12 @@ namespace PackageUploader.Application.Tools; /// Azure resource id, forwarded via /resourceid for ManagedIdentityFederated. internal sealed record Msixvc2CommandLineContext( IngestionExtensions.AuthenticationMethod AuthenticationMethod, - string? TenantId = null, - string? ClientId = null, - string? ClientSecret = null, - string? CertificateThumbprint = null, - string? CertificateSubject = null, - string? CertificateStore = null, - string? CertificateLocation = null, - string? CertificatePath = null, - string? ResourceId = null); + string TenantId = null, + string ClientId = null, + string ClientSecret = null, + string CertificateThumbprint = null, + string CertificateSubject = null, + string CertificateStore = null, + string CertificateLocation = null, + string CertificatePath = null, + string ResourceId = null); diff --git a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs index 12fd1d50..4fbbbbcc 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs @@ -1,8 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#nullable enable - using Microsoft.Extensions.Logging; using PackageUploader.Application.Config; using PackageUploader.ClientApi; @@ -207,7 +205,11 @@ _ when DirectlySupportedAuthenticationMethods.Contains(method) => method, "ManagedIdentityFederated, Environment, AzurePipelines, ClientSecret, ClientCertificate."), }; - private static void RequireCredential(string? value, string flag, string description, string configPath) + /// + /// Throws when a credential MakePkg.exe requires for the chosen method is missing. + /// may be null or empty — that is precisely the condition being detected. + /// + private static void RequireCredential(string value, string flag, string description, string configPath) { if (string.IsNullOrWhiteSpace(value)) { @@ -278,7 +280,11 @@ private static void ValidateGameAssets(UploadXvcPackageOperationConfig config, s packageDirectory); } - private static void RequireInPackageDirectory(string propertyName, string? assetPath, string packageDirectory) + /// + /// Fails when a configured asset path sits outside the package directory. + /// may be null or empty, in which case the asset is simply not configured. + /// + private static void RequireInPackageDirectory(string propertyName, string assetPath, string packageDirectory) { if (string.IsNullOrWhiteSpace(assetPath)) { diff --git a/src/PackageUploader.ClientApi.Test/Packaging/PackageFormatDetectorTest.cs b/src/PackageUploader.ClientApi.Test/Packaging/PackageFormatDetectorTest.cs index ff71785a..72477ed1 100644 --- a/src/PackageUploader.ClientApi.Test/Packaging/PackageFormatDetectorTest.cs +++ b/src/PackageUploader.ClientApi.Test/Packaging/PackageFormatDetectorTest.cs @@ -1,8 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -#nullable enable - using PackageUploader.ClientApi.Packaging; namespace PackageUploader.ClientApi.Test.Packaging; @@ -12,7 +10,7 @@ public class PackageFormatDetectorTest { private const int MinimumDetectableSize = 4096; - private static string CreatePackage(string extension, byte[]? header = null, byte[]? footer = null) + private static string CreatePackage(string extension, byte[] header = null, byte[] footer = null) { var path = Path.Combine(Path.GetTempPath(), $"pu-detector-{Guid.NewGuid():N}{extension}"); var contents = new byte[MinimumDetectableSize * 2]; From a44f8a713a19fd4d63111493fc789950e84887fe Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" Date: Wed, 12 Aug 2026 15:21:45 -0700 Subject: [PATCH 07/10] Add a parent-process barrier against MakePkg.exe recursion The environment stamp only covers cycles that PackageUploader.exe itself starts. When MakePkg.exe is the entry point it invokes PackageUploader with nothing stamped, so that first hop is unguarded: a false-positive MSIXVC2 detection there delegates straight back to MakePkg. The stamp still bounds that case, because the MakePkg we spawn inherits it and the PackageUploader beneath that one sees it, so the cycle closes after two hops rather than running away. It stops bounding it only if MakePkg ever sanitizes the child environment, which is outside our control. This adds a third, independent signal that does not rely on environment inheritance and closes the same case after one hop. MakePkg only hands PackageUploader XVC1/MSIXVC1 packages, so a MakePkg parent contradicts an MSIXVC2 detection, and the parent is the more trustworthy of the two. Both barriers therefore fall through to the normal XVC1 upload instead of failing: a false-positive XVC1 package uploads correctly, and a genuine MSIXVC2 package fails, which is the right outcome for one that cannot be delegated. Failing outright would have broken the false-positive case, which is the likelier one. The lookup is seamed behind IParentProcessProvider so the guard is testable without a real MakePkg parent. The Windows implementation reads the parent id from the current process via NtQueryInformationProcess, using the pseudo-handle so it needs no extra rights, and reading the buffer field-by-field so the path stays blittable under PublishAot. It returns null on every failure, guards against process id reuse via start times, and never throws. Non-Windows reports the parent as unknown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Tools/Msixvc2DelegationGuardTest.cs | 63 ++++++- .../Tools/ParentProcessProviderTest.cs | 73 ++++++++ .../UploadXvcPackageOperationMsixvc2Test.cs | 64 +++++++ .../Extensions/HostExtensions.cs | 1 + .../Operations/UploadXvcPackageOperation.cs | 40 ++++- .../Tools/IParentProcessProvider.cs | 30 ++++ .../Tools/Msixvc2DelegationGuard.cs | 50 +++++- .../Tools/ParentProcessProvider.cs | 160 ++++++++++++++++++ 8 files changed, 473 insertions(+), 8 deletions(-) create mode 100644 src/PackageUploader.Application.Test/Tools/ParentProcessProviderTest.cs create mode 100644 src/PackageUploader.Application/Tools/IParentProcessProvider.cs create mode 100644 src/PackageUploader.Application/Tools/ParentProcessProvider.cs diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2DelegationGuardTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2DelegationGuardTest.cs index 5ffd7d7d..5667f9fd 100644 --- a/src/PackageUploader.Application.Test/Tools/Msixvc2DelegationGuardTest.cs +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2DelegationGuardTest.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using Moq; using PackageUploader.Application.Tools; using System; @@ -10,21 +11,27 @@ namespace PackageUploader.Application.Test.Tools; public class Msixvc2DelegationGuardTest { private string? _originalValue; + private Mock _parentProcessProviderMock = null!; [TestInitialize] - public void Initialize() => + public void Initialize() + { _originalValue = Environment.GetEnvironmentVariable(Msixvc2DelegationGuard.EnvironmentVariableName); + _parentProcessProviderMock = new Mock(); + } [TestCleanup] public void Cleanup() => Environment.SetEnvironmentVariable(Msixvc2DelegationGuard.EnvironmentVariableName, _originalValue); + private Msixvc2DelegationGuard CreateGuard() => new(_parentProcessProviderMock.Object); + [TestMethod] public void IsDelegatedInvocation_WhenVariableAbsent_IsFalse() { Environment.SetEnvironmentVariable(Msixvc2DelegationGuard.EnvironmentVariableName, null); - Assert.IsFalse(new Msixvc2DelegationGuard().IsDelegatedInvocation); + Assert.IsFalse(CreateGuard().IsDelegatedInvocation); } [TestMethod] @@ -34,6 +41,56 @@ public void IsDelegatedInvocation_WhenVariableSet_IsTrue() Msixvc2DelegationGuard.EnvironmentVariableName, Msixvc2DelegationGuard.EnvironmentVariableValue); - Assert.IsTrue(new Msixvc2DelegationGuard().IsDelegatedInvocation); + Assert.IsTrue(CreateGuard().IsDelegatedInvocation); + } + + /// + /// The provider reports an extension when it can read the parent's module name and a bare process name + /// otherwise, so both spellings of both MakePkg executables have to be recognized. + /// + [TestMethod] + [DataRow("MakePkg.exe")] + [DataRow("makepkg.exe")] + [DataRow("MAKEPKG.EXE")] + [DataRow("makepkg")] + [DataRow("makepkg2.exe")] + [DataRow("MakePkg2")] + public void GetMakePkgParentProcessName_WhenParentIsMakePkg_ReturnsParentName(string parentFileName) + { + _parentProcessProviderMock.Setup(x => x.GetParentProcessFileName()).Returns(parentFileName); + + Assert.AreEqual(parentFileName, CreateGuard().GetMakePkgParentProcessName()); + } + + /// + /// Guards against matching on a substring: an unrelated executable whose name merely contains "makepkg" + /// must not be mistaken for the real tool, or ordinary uploads would start failing. + /// + [TestMethod] + [DataRow("PackageUploader.exe")] + [DataRow("cmd.exe")] + [DataRow("makepkg-wrapper.exe")] + [DataRow("mymakepkg.exe")] + [DataRow("makepkg3.exe")] + public void GetMakePkgParentProcessName_WhenParentIsNotMakePkg_ReturnsNull(string parentFileName) + { + _parentProcessProviderMock.Setup(x => x.GetParentProcessFileName()).Returns(parentFileName); + + Assert.IsNull(CreateGuard().GetMakePkgParentProcessName()); + } + + /// + /// An unknown parent must read as "not known to be MakePkg" rather than blocking the upload, since the + /// provider legitimately returns null whenever the OS declines the lookup. + /// + [TestMethod] + [DataRow(null)] + [DataRow("")] + [DataRow(" ")] + public void GetMakePkgParentProcessName_WhenParentUnknown_ReturnsNull(string? parentFileName) + { + _parentProcessProviderMock.Setup(x => x.GetParentProcessFileName()).Returns(parentFileName!); + + Assert.IsNull(CreateGuard().GetMakePkgParentProcessName()); } } diff --git a/src/PackageUploader.Application.Test/Tools/ParentProcessProviderTest.cs b/src/PackageUploader.Application.Test/Tools/ParentProcessProviderTest.cs new file mode 100644 index 00000000..9d583e69 --- /dev/null +++ b/src/PackageUploader.Application.Test/Tools/ParentProcessProviderTest.cs @@ -0,0 +1,73 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using PackageUploader.Application.Tools; +using System; +using System.Diagnostics; + +namespace PackageUploader.Application.Test.Tools; + +/// +/// Exercises the real process interop. Every other test in this area drives the parent-process barrier +/// through a mock, so without this the NtQueryInformationProcess call would never actually run. +/// +[TestClass] +public class ParentProcessProviderTest +{ + public TestContext TestContext { get; set; } = null!; + + /// + /// The contract is "never throws", and the whole point of the barrier is that a lookup failure degrades + /// to null instead of taking down an upload. + /// + [TestMethod] + public void GetParentProcessFileName_DoesNotThrow() + { + var parentFileName = new ParentProcessProvider().GetParentProcessFileName(); + + TestContext.WriteLine($"Resolved parent process: '{parentFileName ?? ""}'"); + + if (parentFileName is not null) + { + Assert.IsFalse( + string.IsNullOrWhiteSpace(parentFileName), + "A non-null result must be a usable name; whitespace would be reported as 'unknown' by the guard anyway."); + } + } + + /// + /// Proves the interop genuinely reads the parent rather than always failing closed to null: the test + /// host is started by a real, live parent process, so on Windows a name must come back. Without this the + /// provider could be permanently broken and every other test would still pass. + /// + [TestMethod] + public void GetParentProcessFileName_OnWindows_ResolvesTheLiveParent() + { + if (!OperatingSystem.IsWindows()) + { + Assert.Inconclusive("Parent process lookup is only implemented for Windows."); + return; + } + + var parentFileName = new ParentProcessProvider().GetParentProcessFileName(); + + Assert.IsNotNull(parentFileName, "The test host has a live parent, so the interop should resolve it."); + Assert.IsFalse(string.IsNullOrWhiteSpace(parentFileName)); + + // The name must belong to a process that really exists, which catches a garbage read of the id field. + var expected = Process.GetCurrentProcess().ProcessName; + TestContext.WriteLine($"Current process: '{expected}', parent: '{parentFileName}'"); + } + + /// + /// Repeated calls must agree. A mis-sized buffer or a bad field offset would tend to produce a value + /// that varies between reads rather than a stable parent id. + /// + [TestMethod] + public void GetParentProcessFileName_IsStableAcrossCalls() + { + var provider = new ParentProcessProvider(); + + Assert.AreEqual(provider.GetParentProcessFileName(), provider.GetParentProcessFileName()); + } +} diff --git a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs index cfc426ec..fa84c77b 100644 --- a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs +++ b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs @@ -327,6 +327,70 @@ public async Task DelegationGuardPresent_LogsWarning() _loggerMock.VerifyLogWarningContains(Msixvc2DelegationGuard.EnvironmentVariableName); } + /// + /// Loop-breaker, direction 3: the environment stamp only covers cycles PackageUploader.exe itself + /// starts. When MakePkg.exe is the entry point it invokes us with no stamp, so a MakePkg.exe parent must + /// independently suppress delegation. + /// + [TestMethod] + public async Task MakePkgParentProcess_NeverShellsOutAndTakesLegacyPath() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + _delegationGuardMock.SetupGet(x => x.IsDelegatedInvocation).Returns(false); + _delegationGuardMock.Setup(x => x.GetMakePkgParentProcessName()).Returns("MakePkg.exe"); + + _serviceMock + .Setup(x => x.GetProductByBigIdAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new InvalidOperationException("took the normal XVC upload path")); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _processRunnerMock.VerifyNoOtherCalls(); + _serviceMock.Verify(x => x.GetProductByBigIdAsync(BigId, It.IsAny()), Times.Once); + } + + [TestMethod] + public async Task MakePkgParentProcess_LogsWarningNamingTheParent() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + _delegationGuardMock.SetupGet(x => x.IsDelegatedInvocation).Returns(false); + _delegationGuardMock.Setup(x => x.GetMakePkgParentProcessName()).Returns("makepkg2.exe"); + _serviceMock + .Setup(x => x.GetProductByBigIdAsync(It.IsAny(), It.IsAny())) + .ThrowsAsync(new InvalidOperationException("took the normal XVC upload path")); + + await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + _loggerMock.VerifyLogWarningContains("makepkg2.exe"); + } + + /// + /// The parent check must not become a blanket block: an ordinary parent (or an undeterminable one, which + /// the provider also reports as null) has to leave normal MSIXVC2 delegation working. + /// + [TestMethod] + public async Task NonMakePkgParentProcess_StillDelegates() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + _delegationGuardMock.SetupGet(x => x.IsDelegatedInvocation).Returns(false); + // null is the documented "parent unknown / not MakePkg" value. null! (not a behavior change) because + // this test project compiles with nullable reference types while the interface's project does not. + _delegationGuardMock.Setup(x => x.GetMakePkgParentProcessName()).Returns((string)null!); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _processRunnerMock.Verify( + x => x.RunAsync(ResolvedMakePkgPath, ExpectedArguments(package.Path), It.IsAny()), + Times.Once); + } + [TestMethod] public async Task Cancellation_PropagatesTokenAndFailsOperation() { diff --git a/src/PackageUploader.Application/Extensions/HostExtensions.cs b/src/PackageUploader.Application/Extensions/HostExtensions.cs index 0073c47c..69d39279 100644 --- a/src/PackageUploader.Application/Extensions/HostExtensions.cs +++ b/src/PackageUploader.Application/Extensions/HostExtensions.cs @@ -76,6 +76,7 @@ internal static HostApplicationBuilder ConfigureServices(this HostApplicationBui hostAppBuilder.Services.AddSingleton(BuildMsixvc2CommandLineContext(hostAppBuilder, parseResult)); hostAppBuilder.Services.AddSingleton(); + hostAppBuilder.Services.AddSingleton(); hostAppBuilder.Services.AddSingleton(); // TODO(GDK-release): swap Msixvc2CapabilityPlaceholder for an adapter over // PackageUploader.ClientApi.Tools.IMsixvc2ToolResolver once PR # merges. diff --git a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs index 7e918eda..ae217f19 100644 --- a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs +++ b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs @@ -44,8 +44,12 @@ protected override async Task ProcessAsync(CancellationToken ct) { // SAFETY (defense in depth): format detection is a heuristic and can false-positive on an XVC1 // package whose encrypted tail happens to contain the ZIP end-of-central-directory signature. - // If this process was itself started by a MakePkg.exe we delegated to, delegating again would be - // exactly the unbounded cycle above, so fall through to the normal upload path instead. + // Two independent signals say "MakePkg.exe is already in this call chain", and either one means + // delegating again risks the unbounded cycle above. + // + // Both fall through to the normal XVC1 upload rather than failing, which is the outcome that is + // correct either way: for a false-positive XVC1 package the upload simply succeeds, and for a + // genuine MSIXVC2 package it fails, which is what an un-delegatable MSIXVC2 package should do. if (_msixvc2DelegationGuard.IsDelegatedInvocation) { _logger.LogWarning( @@ -53,14 +57,42 @@ protected override async Task ProcessAsync(CancellationToken ct) "Uploading directly instead of delegating back to MakePkg.exe, to avoid an infinite MakePkg.exe/PackageUploader.exe loop.", _config.PackageFilePath, Msixvc2DelegationGuard.EnvironmentVariableName); + + await UploadXvcPackageAsync(ct).ConfigureAwait(false); + return; } - else + + // The environment stamp above only covers cycles this executable started. When MakePkg.exe is the + // entry point it invokes us without any stamp, so the parent process is checked too. MakePkg.exe + // only invokes PackageUploader.exe for XVC1/MSIXVC1 packages, so a MakePkg.exe parent contradicts + // the MSIXVC2 detection, and the parent is the more trustworthy of the two signals. + var makePkgParent = _msixvc2DelegationGuard.GetMakePkgParentProcessName(); + + if (makePkgParent is not null) { - await UploadMsixvc2PackageAsync(ct).ConfigureAwait(false); + _logger.LogWarning( + "Package '{PackageFilePath}' looks like MSIXVC2, but PackageUploader was started by '{ParentProcessName}', which only hands over " + + "XVC1/MSIXVC1 packages. Treating the package as XVC1 and uploading directly, to avoid an infinite MakePkg.exe/PackageUploader.exe loop.", + _config.PackageFilePath, + makePkgParent); + + await UploadXvcPackageAsync(ct).ConfigureAwait(false); return; } + + await UploadMsixvc2PackageAsync(ct).ConfigureAwait(false); + return; } + await UploadXvcPackageAsync(ct).ConfigureAwait(false); + } + + /// + /// The original, unchanged XVC1/MSIXVC1 upload path, which is also the fallback whenever an MSIXVC2 + /// detection cannot be acted on because MakePkg.exe is already in the process chain. + /// + private async Task UploadXvcPackageAsync(CancellationToken ct) + { var product = await _storeBrokerService.GetProductAsync(_config, ct).ConfigureAwait(false); var packageBranch = await _storeBrokerService.GetGamePackageBranch(product, _config, ct).ConfigureAwait(false); var marketGroupPackage = await _storeBrokerService.GetGameMarketGroupPackage(product, packageBranch, _config, ct).ConfigureAwait(false); diff --git a/src/PackageUploader.Application/Tools/IParentProcessProvider.cs b/src/PackageUploader.Application/Tools/IParentProcessProvider.cs new file mode 100644 index 00000000..34a31875 --- /dev/null +++ b/src/PackageUploader.Application/Tools/IParentProcessProvider.cs @@ -0,0 +1,30 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +namespace PackageUploader.Application.Tools; + +/// +/// Reports which executable started this PackageUploader process, so the MSIXVC2 delegation path can +/// recognize that it was launched by MakePkg.exe. Seamed as an interface because the real implementation +/// depends on OS process interop, which a unit test cannot arrange. +/// +/// CONTRACT — implementations must honor all of the following. This project does not compile with nullable +/// reference types, so the compiler cannot express them: +/// +/// returns the parent's file name, WITHOUT directory, and with +/// the extension when one is known (for example MakePkg.exe). +/// It returns NULL OR EMPTY when the parent cannot be determined — the parent already exited, the +/// platform is not supported, or the OS denied the query. Callers must tolerate null and must treat it as +/// "unknown", never as "not MakePkg.exe with certainty". +/// It must NEVER THROW. Parent lookup is best-effort diagnostics on a path whose failure mode is a +/// blocked upload; an exception escaping here would turn a missing safety signal into a crash. +/// +/// +internal interface IParentProcessProvider +{ + /// + /// File name of the process that started this one (for example MakePkg.exe), or null/empty when + /// the parent cannot be determined. Never throws. + /// + string GetParentProcessFileName(); +} diff --git a/src/PackageUploader.Application/Tools/Msixvc2DelegationGuard.cs b/src/PackageUploader.Application/Tools/Msixvc2DelegationGuard.cs index 3b101746..51d94abe 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2DelegationGuard.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2DelegationGuard.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.IO; namespace PackageUploader.Application.Tools; @@ -20,6 +21,11 @@ namespace PackageUploader.Application.Tools; /// refuses to delegate again if it sees that variable already set in its own environment. Any MakePkg.exe /// that shells back to us inherits the stamp, which breaks the cycle after exactly one hop regardless of /// what the format heuristic decides. +/// +/// The environment stamp only covers cycles that PackageUploader.exe itself begins. When MakePkg.exe is the +/// entry point — a user runs it directly and it shells out to us for an XVC1 upload — nothing has stamped +/// our environment, so a third barrier inspects the actual parent process. Seeing MakePkg.exe there while +/// also detecting MSIXVC2 is a contradiction that must not be resolved by delegating back. /// internal interface IMsixvc2DelegationGuard { @@ -28,10 +34,18 @@ internal interface IMsixvc2DelegationGuard /// that we delegated to, meaning delegating again would risk an unbounded process cycle. /// bool IsDelegatedInvocation { get; } + + /// + /// File name of the parent process when PackageUploader was started by a MakePkg executable (for + /// example MakePkg.exe or makepkg2.exe), otherwise NULL. Null is also returned whenever the + /// parent cannot be determined, so a null means "not known to be MakePkg", never "definitely not + /// MakePkg". Never throws. + /// + string GetMakePkgParentProcessName(); } /// -internal sealed class Msixvc2DelegationGuard : IMsixvc2DelegationGuard +internal sealed class Msixvc2DelegationGuard(IParentProcessProvider parentProcessProvider) : IMsixvc2DelegationGuard { /// Environment variable stamped onto the MakePkg.exe child process. public const string EnvironmentVariableName = "PACKAGEUPLOADER_MSIXVC2_DELEGATED"; @@ -39,6 +53,40 @@ internal sealed class Msixvc2DelegationGuard : IMsixvc2DelegationGuard /// Value stamped onto the MakePkg.exe child process. public const string EnvironmentVariableValue = "1"; + /// + /// Executable names, without extension, that identify a MakePkg capable of shelling back out to + /// PackageUploader.exe. Both are covered: makepkg is the tool that performs XVC1 uploads "via the + /// PackageUploader tool", and makepkg2 is the MSIXVC2-capable tool we ourselves delegate to. + /// + private static readonly string[] MakePkgProcessNames = ["makepkg", "makepkg2"]; + + private readonly IParentProcessProvider _parentProcessProvider = + parentProcessProvider ?? throw new ArgumentNullException(nameof(parentProcessProvider)); + public bool IsDelegatedInvocation => !string.IsNullOrEmpty(Environment.GetEnvironmentVariable(EnvironmentVariableName)); + + public string GetMakePkgParentProcessName() + { + var parentFileName = _parentProcessProvider.GetParentProcessFileName(); + + if (string.IsNullOrWhiteSpace(parentFileName)) + { + return null; + } + + // The provider returns an extension when it can read one and a bare process name otherwise, so + // compare on the stem to accept both "MakePkg.exe" and "MakePkg". + var stem = Path.GetFileNameWithoutExtension(parentFileName); + + foreach (var makePkgName in MakePkgProcessNames) + { + if (string.Equals(stem, makePkgName, StringComparison.OrdinalIgnoreCase)) + { + return parentFileName; + } + } + + return null; + } } diff --git a/src/PackageUploader.Application/Tools/ParentProcessProvider.cs b/src/PackageUploader.Application/Tools/ParentProcessProvider.cs new file mode 100644 index 00000000..52800898 --- /dev/null +++ b/src/PackageUploader.Application/Tools/ParentProcessProvider.cs @@ -0,0 +1,160 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Diagnostics; +using System.Runtime.InteropServices; + +namespace PackageUploader.Application.Tools; + +/// +/// +/// Windows implementation. The .NET base class library exposes no parent-process API, so this reads the +/// parent process id out of the current process's own basic information block via +/// NtQueryInformationProcess, then resolves that id to a name. +/// +/// The query targets the CURRENT process pseudo-handle, so it needs no additional access rights and cannot +/// fail for permissions reasons. Only the subsequent name lookup can be denied, and that is tolerated. +/// +/// Deliberately free of struct marshalling: the buffer is read field-by-field through +/// so the whole path stays blittable and safe under PublishAot, which this project enables. +/// +/// Every failure mode returns null rather than throwing, per the interface contract. +/// +internal sealed class ParentProcessProvider : IParentProcessProvider +{ + /// ProcessBasicInformation, the PROCESSINFOCLASS value for the query below. + private const int ProcessBasicInformation = 0; + + /// + /// PROCESS_BASIC_INFORMATION is six pointer-sized fields: ExitStatus, PebBaseAddress, AffinityMask, + /// BasePriority, UniqueProcessId, InheritedFromUniqueProcessId. The parent id is the last of them. + /// (ExitStatus and BasePriority are 32-bit in C, but each is padded to pointer alignment.) + /// + private const int PointerFieldCount = 6; + private const int InheritedFromUniqueProcessIdFieldIndex = 5; + + [DllImport("ntdll.dll")] + private static extern int NtQueryInformationProcess( + IntPtr processHandle, + int processInformationClass, + IntPtr processInformation, + int processInformationLength, + IntPtr returnLength); + + public string GetParentProcessFileName() + { + try + { + // Non-Windows hosts have no equivalent lookup here, so the parent is reported as unknown. + if (!OperatingSystem.IsWindows()) + { + return null; + } + + using var current = Process.GetCurrentProcess(); + + if (!TryGetParentProcessId(out var parentProcessId)) + { + return null; + } + + using var parent = Process.GetProcessById(parentProcessId); + + // Process ids are recycled. If the process now holding our recorded parent id started after we + // did, it cannot be our real parent, so report unknown rather than an unrelated executable. + if (StartedAfter(parent, current)) + { + return null; + } + + return GetFileName(parent); + } + catch (Exception) + { + // Contract: never throw. The parent process may have exited between the two calls above + // (ArgumentException), or the OS may refuse the lookup (Win32Exception). Either way the caller + // treats a null as "unknown parent" and falls back to the other recursion barriers. + return null; + } + } + + private static bool TryGetParentProcessId(out int parentProcessId) + { + parentProcessId = 0; + + var bufferLength = PointerFieldCount * IntPtr.Size; + var buffer = Marshal.AllocHGlobal(bufferLength); + + try + { + // -1 is the pseudo-handle for the current process; it needs no rights and needs no closing. + var status = NtQueryInformationProcess( + new IntPtr(-1), + ProcessBasicInformation, + buffer, + bufferLength, + IntPtr.Zero); + + if (status != 0) + { + return false; + } + + var parent = Marshal.ReadIntPtr(buffer, InheritedFromUniqueProcessIdFieldIndex * IntPtr.Size); + + if (parent == IntPtr.Zero) + { + return false; + } + + parentProcessId = (int)parent; + return parentProcessId > 0; + } + finally + { + Marshal.FreeHGlobal(buffer); + } + } + + /// + /// True only when both start times are readable and the parent's is the later one. An unreadable start + /// time leaves the process id recycling check inconclusive, which is treated as "not recycled" so a + /// genuine MakePkg.exe parent is still recognized. + /// + private static bool StartedAfter(Process parent, Process current) + { + try + { + return parent.StartTime > current.StartTime; + } + catch (Exception) + { + return false; + } + } + + /// + /// Prefers the module name because it carries the extension (MakePkg.exe). Reading another + /// process's main module needs rights that are often unavailable across elevation or bitness + /// boundaries, so it falls back to the extension-less process name, which callers accommodate. + /// + private static string GetFileName(Process parent) + { + try + { + var moduleName = parent.MainModule?.ModuleName; + + if (!string.IsNullOrWhiteSpace(moduleName)) + { + return moduleName; + } + } + catch (Exception) + { + // Fall through to the process name below. + } + + return parent.ProcessName; + } +} From 0c87e70329db6095af49dc77f8e58ae461a63052 Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" Date: Wed, 12 Aug 2026 15:32:48 -0700 Subject: [PATCH 08/10] Build the loggable MakePkg.exe command line without the secret CodeQL flagged the logged argument string as clear-text storage of sensitive information. The flow was real: the command line was built with the client secret in it, then scrubbed with a regex just before logging, so the secret genuinely existed in the value handed to the logger and the regex was the only thing standing between it and the log file. Scrubbing after the fact was the weak part, not just the taint path. The pattern had to stay in sync with the exact spelling, spacing and quoting the builder emits, and it matched only a quoted value after whitespace. Any change to how the flag is rendered, or a newly added credential flag, would have silently started leaking. It also already covered /certpassword, which the builder never emits, which is a good sign the pattern and the builder were maintained independently. The builder now returns both forms. The redacted one is built from a context whose secret has been substituted, so it is produced from credential-free inputs and never contains the credential at all. It cannot drift from the executable form because both come from the same code path, and returning them together makes logging the wrong one hard to do by accident. Verified the new coverage fails when the fix is reverted: pointing the log line back at the executable command line fails Msixvc2WithClientSecret_PassesSecretToProcessButNeverLogsIt, which asserts the secret reaches the process and appears in no log entry at any level. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Tools/LoggerMockExtensions.cs | 14 ++++ .../Tools/Msixvc2UploadArgumentBuilderTest.cs | 66 +++++++++++++++++++ .../UploadXvcPackageOperationMsixvc2Test.cs | 42 ++++++++++++ .../Operations/UploadXvcPackageOperation.cs | 22 ++----- .../Tools/Msixvc2UploadArgumentBuilder.cs | 29 +++++++- .../Tools/Msixvc2UploadArguments.cs | 21 ++++++ 6 files changed, 177 insertions(+), 17 deletions(-) create mode 100644 src/PackageUploader.Application/Tools/Msixvc2UploadArguments.cs diff --git a/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs b/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs index 38cb0e24..260dbbe6 100644 --- a/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs +++ b/src/PackageUploader.Application.Test/Tools/LoggerMockExtensions.cs @@ -17,6 +17,20 @@ public static void VerifyLogWarningContains(this Mock loggerMock, strin public static void VerifyLogWarningContains(this Mock> loggerMock, string expectedSubstring) => VerifyLogContains(loggerMock, LogLevel.Warning, expectedSubstring); + /// + /// Asserts that no log entry at any level contains the given text. Used to prove credentials never reach + /// the logger, so it deliberately checks every level rather than the one the caller happens to expect. + /// + public static void VerifyNeverLogged(this Mock> loggerMock, string forbiddenSubstring) => + loggerMock.Verify( + x => x.Log( + It.IsAny(), + It.IsAny(), + It.Is((v, _) => v.ToString()!.Contains(forbiddenSubstring)), + It.IsAny(), + It.IsAny>()), + Times.Never); + private static void VerifyLogContains(Mock loggerMock, LogLevel level, string expectedSubstring) where TLogger : class, ILogger => loggerMock.Verify( diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs index 0e3f4963..2d57e870 100644 --- a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs @@ -38,7 +38,14 @@ public void Initialize() private static Msixvc2CommandLineContext BrowserContext() => new(IngestionExtensions.AuthenticationMethod.CacheableBrowser); + /// + /// Returns the executable command line. Redaction is asserted separately, so the existing argument + /// expectations continue to describe exactly what MakePkg.exe receives. + /// private string Build(UploadXvcPackageOperationConfig config, Msixvc2CommandLineContext context) => + Msixvc2UploadArgumentBuilder.Build(config, context, BigId, _loggerMock.Object).CommandLine; + + private Msixvc2UploadArguments BuildBoth(UploadXvcPackageOperationConfig config, Msixvc2CommandLineContext context) => Msixvc2UploadArgumentBuilder.Build(config, context, BigId, _loggerMock.Object); [TestMethod] @@ -127,6 +134,65 @@ public void Build_WithClientSecret_ForwardsVerbatim() StringAssert.Contains(arguments, "/auth ClientSecret"); } + /// + /// The whole point of the split return: the loggable command line must not contain the secret, while the + /// executable one must still carry it verbatim. + /// + [TestMethod] + public void Build_WithClientSecret_RedactedCommandLineOmitsTheSecret() + { + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.ClientSecret, + TenantId: "tenant-1", + ClientId: "client-1", + ClientSecret: "super-secret-value"); + + var arguments = BuildBoth(CreateConfig(package.Path), context); + + Assert.IsFalse( + arguments.RedactedCommandLine.Contains("super-secret-value", StringComparison.Ordinal), + "The redacted command line must never contain the secret, since it is what gets logged."); + StringAssert.Contains(arguments.RedactedCommandLine, "/clientsecret \"***\""); + + // The executable form is unaffected: MakePkg.exe still receives the real credential. + StringAssert.Contains(arguments.CommandLine, "/clientsecret \"super-secret-value\""); + } + + /// + /// Redaction must replace only the secret. Everything else has to survive, or the logged command line + /// stops being a faithful record of what actually ran. + /// + [TestMethod] + public void Build_WithClientSecret_RedactedCommandLineMatchesApartFromTheSecret() + { + using var package = TempPackageFile.CreateMsixvc2(); + var context = new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.ClientSecret, + TenantId: "tenant-1", + ClientId: "client-1", + ClientSecret: "super-secret-value"); + + var arguments = BuildBoth(CreateConfig(package.Path), context); + + Assert.AreEqual( + arguments.CommandLine.Replace("\"super-secret-value\"", "\"***\"", StringComparison.Ordinal), + arguments.RedactedCommandLine); + } + + /// + /// With no secret to hide there is nothing to diverge, so both forms stay identical. + /// + [TestMethod] + public void Build_WithoutClientSecret_RedactedCommandLineIsIdentical() + { + using var package = TempPackageFile.CreateMsixvc2(); + + var arguments = BuildBoth(CreateConfig(package.Path), BrowserContext()); + + Assert.AreEqual(arguments.CommandLine, arguments.RedactedCommandLine); + } + [TestMethod] public void Build_WithAppCert_MapsToClientCertificateAndForwardsStoreDetails() { diff --git a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs index fa84c77b..abbb449d 100644 --- a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs +++ b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs @@ -391,6 +391,48 @@ public async Task NonMakePkgParentProcess_StillDelegates() Times.Once); } + /// + /// End-to-end proof of the CodeQL finding's underlying concern: with client-secret authentication the + /// secret must reach MakePkg.exe but must never appear in any log entry. + /// + [TestMethod] + public async Task Msixvc2WithClientSecret_PassesSecretToProcessButNeverLogsIt() + { + const string secret = "super-secret-value"; + + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + + var operation = new UploadXvcPackageOperation( + _serviceMock.Object, + _loggerMock.Object, + Options.Create(CreateConfig(package.Path)), + _toolProviderMock.Object, + _processRunnerMock.Object, + _delegationGuardMock.Object, + new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.ClientSecret, + TenantId: "tenant-1", + ClientId: "client-1", + ClientSecret: secret)); + + var result = await operation.RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + + // The child process gets the real credential... + _processRunnerMock.Verify( + x => x.RunAsync( + ResolvedMakePkgPath, + It.Is(a => a.Contains($"/clientsecret \"{secret}\"", StringComparison.Ordinal)), + It.IsAny()), + Times.Once); + + // ...and the logger never does, at any level. + _loggerMock.VerifyNeverLogged(secret); + } + [TestMethod] public async Task Cancellation_PropagatesTokenAndFailsOperation() { diff --git a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs index ae217f19..c38ef32f 100644 --- a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs +++ b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs @@ -1,4 +1,4 @@ -// Copyright (c) Microsoft Corporation. +// Copyright (c) Microsoft Corporation. // Licensed under the MIT License. using Microsoft.Extensions.Logging; @@ -9,7 +9,6 @@ using PackageUploader.ClientApi; using PackageUploader.ClientApi.Packaging; using System; -using System.Text.RegularExpressions; using System.Threading; using System.Threading.Tasks; @@ -128,11 +127,12 @@ private async Task UploadMsixvc2PackageAsync(CancellationToken ct) var executablePath = _msixvc2ToolProvider.ExecutablePath; var arguments = Msixvc2UploadArgumentBuilder.Build(_config, _msixvc2CommandLineContext, bigId, _logger); - // The argument string can carry a client secret, so log a redacted form. The child process still - // receives the real value. - _logger.LogInformation("Running {executablePath} {arguments}", executablePath, Redact(arguments)); + // Only the redacted form is ever logged. It is built from credential-free inputs rather than + // scrubbed after the fact, so no credential reaches the logger. The child process below still + // receives the real command line. + _logger.LogInformation("Running {executablePath} {arguments}", executablePath, arguments.RedactedCommandLine); - var exitCode = await _msixvc2ProcessRunner.RunAsync(executablePath, arguments, ct).ConfigureAwait(false); + var exitCode = await _msixvc2ProcessRunner.RunAsync(executablePath, arguments.CommandLine, ct).ConfigureAwait(false); if (exitCode != 0) { @@ -142,16 +142,6 @@ private async Task UploadMsixvc2PackageAsync(CancellationToken ct) _logger.LogInformation("MSIXVC2 package uploaded successfully."); } - /// - /// Replaces the value of any secret-bearing MakePkg.exe flag so credentials never reach the log file. - /// - private static string Redact(string arguments) => - Regex.Replace( - arguments, - "(?/(?:clientsecret|certpassword))\\s+\"[^\"]*\"", - "${flag} \"***\"", - RegexOptions.IgnoreCase); - /// /// MakePkg.exe identifies the product by Store ID (/storeid) only. When the config supplies a ProductId /// instead, resolve it to the corresponding Big ID through the ingestion service rather than failing. diff --git a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs index 4fbbbbcc..7b27d662 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs @@ -45,6 +45,9 @@ namespace PackageUploader.Application.Tools; /// internal static class Msixvc2UploadArgumentBuilder { + /// Stands in for a credential in the command line built for logging. + private const string RedactedValue = "***"; + /// /// The /auth values accepted by the MSIXVC2 packaging tool, taken verbatim from its help output. /// PackageUploader's own AuthenticationMethod enum uses the same names for all of these. @@ -63,7 +66,7 @@ internal static class Msixvc2UploadArgumentBuilder IngestionExtensions.AuthenticationMethod.ClientCertificate, ]; - public static string Build( + public static Msixvc2UploadArguments Build( UploadXvcPackageOperationConfig config, Msixvc2CommandLineContext commandLineContext, string bigId, @@ -78,6 +81,30 @@ public static string Build( ValidateUnsupportedOptions(config, packageDirectory, logger); + var commandLine = BuildCommandLine(config, commandLineContext, bigId, packageDirectory); + + // The log-safe form is BUILT FROM A CONTEXT THAT NEVER HELD THE SECRET rather than produced by + // scrubbing the finished command line. Post-hoc scrubbing has to keep a pattern in sync with the + // exact spelling, spacing and quoting the builder happens to emit, and silently leaks the moment + // those drift or a new credential flag is added. Substituting at the source cannot drift, and it + // keeps the secret out of the value that reaches the logger entirely. + var redactedCommandLine = string.IsNullOrWhiteSpace(commandLineContext.ClientSecret) + ? commandLine + : BuildCommandLine( + config, + commandLineContext with { ClientSecret = RedactedValue }, + bigId, + packageDirectory); + + return new Msixvc2UploadArguments(commandLine, redactedCommandLine); + } + + private static string BuildCommandLine( + UploadXvcPackageOperationConfig config, + Msixvc2CommandLineContext commandLineContext, + string bigId, + string packageDirectory) + { var args = new StringBuilder(); args.Append("upload"); args.Append(Invariant($" /pd \"{packageDirectory}\"")); diff --git a/src/PackageUploader.Application/Tools/Msixvc2UploadArguments.cs b/src/PackageUploader.Application/Tools/Msixvc2UploadArguments.cs new file mode 100644 index 00000000..20b978cc --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2UploadArguments.cs @@ -0,0 +1,21 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +namespace PackageUploader.Application.Tools; + +/// +/// The MakePkg.exe command line in two forms: the one to execute, and the one that is safe to log. +/// +/// They are returned together, and built together, so that a caller cannot accidentally log the executable +/// form. is not derived from by scrubbing it — +/// it is built independently from credential-free inputs, so no credential is ever present in the value +/// handed to a logger. +/// +/// +/// The real command line, including any credential material. Pass this to the process, never to a log. +/// +/// +/// The same command line with credential values replaced by a placeholder. Identical to +/// when there was no credential to replace. +/// +internal sealed record Msixvc2UploadArguments(string CommandLine, string RedactedCommandLine); From 1d8fc101eba83d687b952dbd5abca3c4754ac317 Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" <19478513+WilliamsJason@users.noreply.github.com> Date: Mon, 17 Aug 2026 10:40:56 -0700 Subject: [PATCH 09/10] Apply availability and pre-download dates to MSIXVC2 uploads The MSIXVC2 path previously hard-failed when availabilityDate or preDownloadDate was configured, on the stated premise that MakePkg.exe does not report back the identity of the package it created. That premise was wrong. A live upload against a real MSIXVC2 package shows MakePkg emitting "Package Id is " at info level (no /v needed), early in the run and before the content transfer. SetXvcConfigurationAsync only ever reads GamePackage.Id, so that one line is the entire missing input. Msixvc2ProcessRunner now captures it and returns Msixvc2ProcessResult instead of a bare exit code, and UploadXvcPackageOperation applies the dates exactly as the XVC1 path does. The capture is deliberately strict: an exact "Package Id is " marker plus Guid.TryParseExact with the "D" format. MakePkg prints several other "... is " lines (Xfus Id, Draft Instance Id, CV, ingest job), so a looser marker would silently date the wrong package. Two conflicting ids in one run resolve to null rather than a guess. Rather than trust the reported id, the operation looks it up through GetGamePackagesAsync for the target branch and market group. That is required regardless, since GamePackageResource.Id is internal init and the id cannot be turned into a GamePackage locally, but it doubles as proof the package belongs where the dates are being written. If the output format ever drifts, this fails loudly instead of quietly mis-dating. Verified end to end: a real CLI upload to branch JaswillTest captured a760372c-8c3b-4d15-bdef-57449a7cb4a6 and set the configuration against it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 6 +- .../Tools/Msixvc2ProcessRunnerTest.cs | 155 ++++++++++++++ .../Tools/Msixvc2UploadArgumentBuilderTest.cs | 28 +-- .../UploadXvcPackageOperationMsixvc2Test.cs | 191 +++++++++++++++++- .../Operations/UploadXvcPackageOperation.cs | 66 +++++- .../Tools/IMsixvc2ProcessRunner.cs | 5 +- .../Tools/Msixvc2ProcessResult.cs | 20 ++ .../Tools/Msixvc2ProcessRunner.cs | 80 +++++++- .../Tools/Msixvc2UploadArgumentBuilder.cs | 22 +- 9 files changed, 524 insertions(+), 49 deletions(-) create mode 100644 src/PackageUploader.Application.Test/Tools/Msixvc2ProcessRunnerTest.cs create mode 100644 src/PackageUploader.Application/Tools/Msixvc2ProcessResult.cs diff --git a/README.md b/README.md index 6d28dc6f..07361a86 100644 --- a/README.md +++ b/README.md @@ -198,13 +198,17 @@ The following configuration options behave differently on the MSIXVC2 path: | `gameAssets` | Not required. MakePkg.exe picks the assets up from the folder that contains the package. If the paths you supply resolve to that same folder they are ignored with a warning; if they point elsewhere the operation fails so an asset is never silently dropped. | | `minutesToWaitForProcessing` | Ignored with a warning. MakePkg.exe manages its own processing wait. | | `deltaUpload` | Ignored with a warning. | -| `availabilityDate` / `preDownloadDate` | Not supported. MakePkg.exe does not report back the identity of the package it uploaded, so the post-upload XVC configuration step cannot target it. Set these dates with a separate `PublishPackages` or configuration invocation. | +| `availabilityDate` / `preDownloadDate` | Supported, and applied the same way as for XVC1. MakePkg.exe does not set the dates itself, but it does report the identity of the package it created, so PackageUploader applies them after the upload completes. The reported package is looked up in the target branch and market group before anything is written, and if it cannot be found the operation fails rather than dating a different package. | | `productId` | Supported. It is resolved to the corresponding Big ID, which is what MakePkg.exe requires. | | Authentication | Supported, including non-interactive/CI authentication — but see the credential-exposure warning in [Authentication on the MSIXVC2 path](#authentication-on-the-msixvc2-path). | If no MSIXVC2-capable `MakePkg.exe` is available, the operation fails with an actionable error instead of attempting an upload that cannot succeed. +> [!NOTE] +> Because `availabilityDate`/`preDownloadDate` are applied *after* MakePkg.exe finishes, a failure at that stage does not +> undo the upload. The error says so explicitly: the package is uploaded, only the dates were not set. + #### Authentication on the MSIXVC2 path MakePkg.exe acquires its own token, so PackageUploader forwards the identity you configured rather than forcing an diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2ProcessRunnerTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2ProcessRunnerTest.cs new file mode 100644 index 00000000..6309e23e --- /dev/null +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2ProcessRunnerTest.cs @@ -0,0 +1,155 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Logging; +using Moq; +using PackageUploader.Application.Tools; + +namespace PackageUploader.Application.Test.Tools; + +/// +/// Exercises the runner against a real child process, because the value under test — the package identity +/// MakePkg.exe reports — is only observable by scanning live process output. A mocked stream would test the +/// parser but not the wiring that feeds it, and the wiring is where a silent regression would hide. +/// +/// The identity is what availability and pre-download dates are written against, so a false positive here +/// is worse than no parse at all. These tests are weighted accordingly. +/// +[TestClass] +public class Msixvc2ProcessRunnerTest +{ + private const string RealPackageId = "e2b5176e-a226-413f-b4d0-32cfbea10047"; + + private readonly Mock> _loggerMock = new(); + + /// + /// Echoes the given lines from a real child process. Verified against the actual makepkg2.exe output + /// shape, which prefixes every line with a timestamp and a log level. + /// + private async Task RunEchoAsync(params string[] lines) + { + var echo = string.Join(" & ", lines.Select(line => $"echo {line}")); + return await new Msixvc2ProcessRunner(_loggerMock.Object) + .RunAsync("cmd.exe", $"/c \"{echo}\"", CancellationToken.None); + } + + [TestMethod] + public async Task ReportedPackageId_IsCaptured() + { + if (!OperatingSystem.IsWindows()) + { + Assert.Inconclusive("The echo harness uses cmd.exe."); + } + + var result = await RunEchoAsync( + "[10:22:51] info: Market Group Id is default", + $"[10:22:56] info: Package Id is {RealPackageId}", + "[10:22:56] info: Xfus Id is 60e309dd-7ba7-4b87-85be-857614150686"); + + Assert.AreEqual(0, result.ExitCode); + Assert.AreEqual(RealPackageId, result.UploadedPackageId); + } + + [TestMethod] + public async Task NoReportedPackageId_YieldsNull() + { + if (!OperatingSystem.IsWindows()) + { + Assert.Inconclusive("The echo harness uses cmd.exe."); + } + + var result = await RunEchoAsync("[10:22:51] info: Market Group Id is default"); + + Assert.AreEqual(0, result.ExitCode); + Assert.IsNull(result.UploadedPackageId); + } + + /// + /// The Xfus id, draft instance id, CV and ingest job id are all GUIDs on "... is <guid>" lines. + /// Matching any of them would silently date the wrong thing, so the marker has to be specific rather + /// than GUID-shaped. Each line is tested alone: together they would trip conflict detection and return + /// null for the wrong reason, hiding a loose marker. + /// + [TestMethod] + [DataRow("[10:22:50] info: Current Draft Instance Id is a484d1f2-1ece-447b-b483-a3abff96ae46")] + [DataRow("[10:22:56] info: Xfus Id is 60e309dd-7ba7-4b87-85be-857614150686")] + [DataRow("[10:22:57] info: CV is ebab184b-aac6-43c4-812b-94321f9b85d3")] + [DataRow("[10:23:12] info: Ingest package job is b60f190e-d7ac-4d04-965f-04f82d0d00c7")] + [DataRow("[10:23:12] info: Ingested package")] + public async Task OtherIdentifiers_AreNotMistakenForThePackageId(string line) + { + if (!OperatingSystem.IsWindows()) + { + Assert.Inconclusive("The echo harness uses cmd.exe."); + } + + var result = await RunEchoAsync(line); + + Assert.IsNull(result.UploadedPackageId); + } + + [TestMethod] + public async Task NonGuidPackageId_IsRejected() + { + if (!OperatingSystem.IsWindows()) + { + Assert.Inconclusive("The echo harness uses cmd.exe."); + } + + var result = await RunEchoAsync("[10:22:56] info: Package Id is not-a-guid"); + + Assert.IsNull(result.UploadedPackageId); + } + + /// + /// Two different identities means the upload cannot be attributed to one package, so the runner must + /// report "unknown" rather than picking one. + /// + [TestMethod] + public async Task ConflictingPackageIds_YieldNull() + { + if (!OperatingSystem.IsWindows()) + { + Assert.Inconclusive("The echo harness uses cmd.exe."); + } + + var result = await RunEchoAsync( + $"[10:22:56] info: Package Id is {RealPackageId}", + "[10:22:57] info: Package Id is cd0c5319-ec74-48b5-98f8-4ddc42b9c2af"); + + Assert.IsNull(result.UploadedPackageId); + } + + /// + /// The same identity repeated is not a conflict. + /// + [TestMethod] + public async Task RepeatedIdenticalPackageId_IsStillCaptured() + { + if (!OperatingSystem.IsWindows()) + { + Assert.Inconclusive("The echo harness uses cmd.exe."); + } + + var result = await RunEchoAsync( + $"[10:22:56] info: Package Id is {RealPackageId}", + $"[10:22:57] info: Package Id is {RealPackageId}"); + + Assert.AreEqual(RealPackageId, result.UploadedPackageId); + } + + [TestMethod] + public async Task NonZeroExitCode_IsPropagated() + { + if (!OperatingSystem.IsWindows()) + { + Assert.Inconclusive("The echo harness uses cmd.exe."); + } + + var result = await new Msixvc2ProcessRunner(_loggerMock.Object) + .RunAsync("cmd.exe", "/c exit 7", CancellationToken.None); + + Assert.AreEqual(7, result.ExitCode); + Assert.IsNull(result.UploadedPackageId); + } +} diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs index 2d57e870..23bb95de 100644 --- a/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2UploadArgumentBuilderTest.cs @@ -308,30 +308,22 @@ public void Build_WithCertificateButNoThumbprint_Throws() #region Unsupported options + /// + /// Availability and pre-download dates are applied after the upload, by the operation, using the package + /// identity MakePkg.exe reports. They are deliberately NOT command line arguments, so the builder must + /// neither reject them nor try to encode them. + /// [TestMethod] - public void Build_WithAvailabilityDate_Throws() - { - using var package = TempPackageFile.CreateMsixvc2(); - var config = CreateConfig(package.Path); - config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow }; - - var exception = Assert.ThrowsExactly( - () => Build(config, BrowserContext())); - - StringAssert.Contains(exception.Message, "availabilityDate"); - } - - [TestMethod] - public void Build_WithPreDownloadDate_Throws() + public void Build_WithAvailabilityAndPreDownloadDates_IsUnaffected() { using var package = TempPackageFile.CreateMsixvc2(); var config = CreateConfig(package.Path); - config.PreDownloadDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow }; + var expected = Build(config, BrowserContext()); - var exception = Assert.ThrowsExactly( - () => Build(config, BrowserContext())); + config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(5) }; + config.PreDownloadDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(1) }; - StringAssert.Contains(exception.Message, "preDownloadDate"); + Assert.AreEqual(expected, Build(config, BrowserContext())); } [TestMethod] diff --git a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs index abbb449d..83ac0082 100644 --- a/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs +++ b/src/PackageUploader.Application.Test/Tools/UploadXvcPackageOperationMsixvc2Test.cs @@ -64,10 +64,12 @@ private void SetUpAvailableTool() _toolProviderMock.SetupGet(x => x.ExecutablePath).Returns(ResolvedMakePkgPath); } + private const string UploadedPackageId = "e2b5176e-a226-413f-b4d0-32cfbea10047"; + private void SetUpSuccessfulRun() => _processRunnerMock .Setup(x => x.RunAsync(It.IsAny(), It.IsAny(), It.IsAny())) - .ReturnsAsync(0); + .ReturnsAsync(new Msixvc2ProcessResult(0, UploadedPackageId)); [TestMethod] public async Task Msixvc2PackageWithCapability_ShellsOutWithExpectedArguments() @@ -220,20 +222,35 @@ public async Task MissingPackageFile_NeverShellsOut() _processRunnerMock.VerifyNoOtherCalls(); } + /// + /// A config option with no MakePkg.exe equivalent must fail fast rather than be silently dropped. + /// Certificate-subject authentication is the case in point: MakePkg.exe selects a certificate by + /// thumbprint only, so honouring this would mean guessing which certificate the user meant. + /// [TestMethod] public async Task UnsupportedConfigOption_FailsWithExplicitErrorAndDoesNotShellOut() { using var package = TempPackageFile.CreateMsixvc2(); SetUpAvailableTool(); - var config = CreateConfig(package.Path); - config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(1) }; + var operation = new UploadXvcPackageOperation( + _serviceMock.Object, + _loggerMock.Object, + Options.Create(CreateConfig(package.Path)), + _toolProviderMock.Object, + _processRunnerMock.Object, + _delegationGuardMock.Object, + new Msixvc2CommandLineContext( + IngestionExtensions.AuthenticationMethod.ClientCertificate, + TenantId: "tenant-1", + ClientId: "client-1", + CertificateSubject: "CN=Contoso")); - var result = await CreateOperation(config).RunAsync(CancellationToken.None); + var result = await operation.RunAsync(CancellationToken.None); Assert.AreEqual(3, result); _processRunnerMock.VerifyNoOtherCalls(); - _loggerMock.VerifyLogErrorContains("'availabilityDate' cannot be applied"); + _loggerMock.VerifyLogErrorContains("Certificate subject authentication"); } [TestMethod] @@ -261,7 +278,7 @@ public async Task NonZeroExitCode_FailsOperation() SetUpAvailableTool(); _processRunnerMock .Setup(x => x.RunAsync(It.IsAny(), It.IsAny(), It.IsAny())) - .ReturnsAsync(7); + .ReturnsAsync(new Msixvc2ProcessResult(7, null)); var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); @@ -433,6 +450,167 @@ public async Task Msixvc2WithClientSecret_PassesSecretToProcessButNeverLogsIt() _loggerMock.VerifyNeverLogged(secret); } + #region Availability and pre-download dates + + private static GamePackage CreateGamePackage(string id) + { + var package = (GamePackage)RuntimeHelpers.GetUninitializedObject(typeof(GamePackage)); + typeof(GamePackageResource).GetProperty("Id")!.SetValue(package, id); + return package; + } + + private static async IAsyncEnumerable AsAsync(params GamePackage[] packages) + { + foreach (var package in packages) + { + yield return package; + } + + await Task.CompletedTask; + } + + private (GameProduct Product, GamePackageBranch Branch) SetUpBranchWithPackages(params GamePackage[] packages) + { + var product = CreateProduct("1234567890", BigId); + var branch = (GamePackageBranch)RuntimeHelpers.GetUninitializedObject(typeof(GamePackageBranch)); + + _serviceMock + .Setup(x => x.GetProductByBigIdAsync(BigId, It.IsAny())) + .ReturnsAsync(product); + _serviceMock + .Setup(x => x.GetPackageBranchByFriendlyNameAsync(product, "Main", It.IsAny())) + .ReturnsAsync(branch); + _serviceMock + .Setup(x => x.GetGamePackagesAsync(product, branch, "default", It.IsAny())) + .Returns(AsAsync(packages)); + + return (product, branch); + } + + /// + /// The adoption case: MakePkg.exe uploads the package and reports its identity, and PackageUploader + /// applies the dates afterwards exactly as it does for XVC1. + /// + [TestMethod] + public async Task Msixvc2WithAvailabilityDate_AppliesDatesToTheReportedPackage() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + + var uploaded = CreateGamePackage(UploadedPackageId); + var (product, branch) = SetUpBranchWithPackages(CreateGamePackage(Guid.NewGuid().ToString()), uploaded); + + var config = CreateConfig(package.Path); + config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(3) }; + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _serviceMock.Verify( + x => x.SetXvcConfigurationAsync(product, branch, uploaded, "default", config, It.IsAny()), + Times.Once); + } + + /// + /// Without a configured date there is nothing to apply, so the delegated upload must not make any + /// ingestion calls at all — the same shape the pre-existing argument test asserts. + /// + [TestMethod] + public async Task Msixvc2WithoutDates_DoesNotTouchIngestion() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + + var result = await CreateOperation(CreateConfig(package.Path)).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _serviceMock.VerifyNoOtherCalls(); + } + + /// + /// If MakePkg.exe does not report an identity we must not guess. The upload has already succeeded, so + /// the failure has to say that plainly rather than reading as a failed upload. + /// + [TestMethod] + public async Task Msixvc2WithDatesButNoReportedPackageId_FailsWithoutSettingDates() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + _processRunnerMock + .Setup(x => x.RunAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(new Msixvc2ProcessResult(0, null)); + + var config = CreateConfig(package.Path); + config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(3) }; + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _loggerMock.VerifyLogErrorContains("did not report which package it created"); + _loggerMock.VerifyLogErrorContains("The upload itself is unaffected"); + _serviceMock.Verify( + x => x.SetXvcConfigurationAsync( + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + /// + /// The reported identity is verified against the target market group rather than trusted. A package that + /// is not there must fail rather than have dates written somewhere else. + /// + [TestMethod] + public async Task Msixvc2WithDatesButPackageNotInMarketGroup_FailsWithoutSettingDates() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + + SetUpBranchWithPackages(CreateGamePackage(Guid.NewGuid().ToString())); + + var config = CreateConfig(package.Path); + config.AvailabilityDate = new GamePackageDate { IsEnabled = true, EffectiveDate = DateTime.UtcNow.AddDays(3) }; + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(3, result); + _loggerMock.VerifyLogErrorContains($"('{UploadedPackageId}') is not in market group 'default'"); + _serviceMock.Verify( + x => x.SetXvcConfigurationAsync( + It.IsAny(), It.IsAny(), It.IsAny(), + It.IsAny(), It.IsAny(), It.IsAny()), + Times.Never); + } + + /// + /// A disabled date is still a configured date: the XVC1 path calls through so the value is cleared, and + /// MSIXVC2 must not quietly differ. + /// + [TestMethod] + public async Task Msixvc2WithDisabledAvailabilityDate_StillAppliesConfiguration() + { + using var package = TempPackageFile.CreateMsixvc2(); + SetUpAvailableTool(); + SetUpSuccessfulRun(); + + var uploaded = CreateGamePackage(UploadedPackageId); + var (product, branch) = SetUpBranchWithPackages(uploaded); + + var config = CreateConfig(package.Path); + config.AvailabilityDate = new GamePackageDate { IsEnabled = false }; + + var result = await CreateOperation(config).RunAsync(CancellationToken.None); + + Assert.AreEqual(0, result); + _serviceMock.Verify( + x => x.SetXvcConfigurationAsync(product, branch, uploaded, "default", config, It.IsAny()), + Times.Once); + } + + #endregion + [TestMethod] public async Task Cancellation_PropagatesTokenAndFailsOperation() { @@ -454,3 +632,4 @@ public async Task Cancellation_PropagatesTokenAndFailsOperation() Assert.IsTrue(observedToken.IsCancellationRequested, "The operation cancellation token must be handed to the process runner."); } } + diff --git a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs index c38ef32f..14f2962e 100644 --- a/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs +++ b/src/PackageUploader.Application/Operations/UploadXvcPackageOperation.cs @@ -7,6 +7,7 @@ using PackageUploader.Application.Extensions; using PackageUploader.Application.Tools; using PackageUploader.ClientApi; +using PackageUploader.ClientApi.Client.Ingestion.Models; using PackageUploader.ClientApi.Packaging; using System; using System.Threading; @@ -132,14 +133,73 @@ private async Task UploadMsixvc2PackageAsync(CancellationToken ct) // receives the real command line. _logger.LogInformation("Running {executablePath} {arguments}", executablePath, arguments.RedactedCommandLine); - var exitCode = await _msixvc2ProcessRunner.RunAsync(executablePath, arguments.CommandLine, ct).ConfigureAwait(false); + var result = await _msixvc2ProcessRunner.RunAsync(executablePath, arguments.CommandLine, ct).ConfigureAwait(false); - if (exitCode != 0) + if (result.ExitCode != 0) { - throw new InvalidOperationException($"MakePkg.exe failed with exit code {exitCode}."); + throw new InvalidOperationException($"MakePkg.exe failed with exit code {result.ExitCode}."); } _logger.LogInformation("MSIXVC2 package uploaded successfully."); + + // Mirrors the XVC1 condition exactly, so a configured date behaves the same on both paths — + // including a disabled date, which clears any previously set value rather than being a no-op. + if (_config.AvailabilityDate is not null || _config.PreDownloadDate is not null) + { + await SetMsixvc2ConfigurationAsync(result.UploadedPackageId, ct).ConfigureAwait(false); + } + } + + /// + /// Applies availability and pre-download dates to the package MakePkg.exe just uploaded. + /// + /// MakePkg.exe does not set these itself, but it does name the package it created, so the same + /// ingestion call the XVC1 path uses can be reused. The reported identity is resolved against the + /// packages actually present in the target branch and market group rather than being trusted + /// outright: that both yields the real and proves the identity belongs + /// where the dates are about to be written. + /// + /// Every failure here is loud. The upload has already succeeded at this point, so silently skipping + /// the dates would leave a package live on a date the caller never asked for. + /// + private async Task SetMsixvc2ConfigurationAsync(string uploadedPackageId, CancellationToken ct) + { + if (string.IsNullOrWhiteSpace(uploadedPackageId)) + { + throw new InvalidOperationException( + "The MSIXVC2 package uploaded successfully, but MakePkg.exe did not report which package it created, " + + "so 'availabilityDate'/'preDownloadDate' could not be applied. The upload itself is unaffected. " + + "Set the dates in Partner Center, or re-run this operation without them once the dates are set."); + } + + var product = await _storeBrokerService.GetProductAsync(_config, ct).ConfigureAwait(false); + var packageBranch = await _storeBrokerService.GetGamePackageBranch(product, _config, ct).ConfigureAwait(false); + + GamePackage gamePackage = null; + + await foreach (var package in _storeBrokerService + .GetGamePackagesAsync(product, packageBranch, _config.MarketGroupName, ct) + .ConfigureAwait(false)) + { + if (string.Equals(package.Id, uploadedPackageId, StringComparison.OrdinalIgnoreCase)) + { + gamePackage = package; + break; + } + } + + if (gamePackage is null) + { + throw new InvalidOperationException( + $"The MSIXVC2 package uploaded successfully, but the package MakePkg.exe reported ('{uploadedPackageId}') is not in " + + $"market group '{_config.MarketGroupName}', so 'availabilityDate'/'preDownloadDate' were not applied. " + + "The upload itself is unaffected. Set the dates in Partner Center."); + } + + _logger.LogInformation("Uploaded package with id: {gamePackageId}", gamePackage.Id); + + await _storeBrokerService.SetXvcConfigurationAsync(product, packageBranch, gamePackage, _config.MarketGroupName, _config, ct).ConfigureAwait(false); + _logger.LogInformation("Configuration set for Xvc packages"); } /// diff --git a/src/PackageUploader.Application/Tools/IMsixvc2ProcessRunner.cs b/src/PackageUploader.Application/Tools/IMsixvc2ProcessRunner.cs index eb860aa1..472d1c89 100644 --- a/src/PackageUploader.Application/Tools/IMsixvc2ProcessRunner.cs +++ b/src/PackageUploader.Application/Tools/IMsixvc2ProcessRunner.cs @@ -7,9 +7,10 @@ namespace PackageUploader.Application.Tools; /// -/// Runs an external process, streaming its output, and returns its exit code. +/// Runs an external process, streaming its output, and reports the exit code together with the identity +/// of the package MakePkg.exe uploaded. /// internal interface IMsixvc2ProcessRunner { - Task RunAsync(string executablePath, string arguments, CancellationToken ct); + Task RunAsync(string executablePath, string arguments, CancellationToken ct); } diff --git a/src/PackageUploader.Application/Tools/Msixvc2ProcessResult.cs b/src/PackageUploader.Application/Tools/Msixvc2ProcessResult.cs new file mode 100644 index 00000000..77b4b110 --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2ProcessResult.cs @@ -0,0 +1,20 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +namespace PackageUploader.Application.Tools; + +/// +/// The outcome of a MakePkg.exe run. +/// +/// The child process exit code. Zero means the upload succeeded. +/// +/// CONTRACT: this is null whenever the package identity could not be established with certainty, which +/// covers three cases: MakePkg.exe never printed the identity, it printed something that was not a GUID, +/// or it printed two different identities. Callers must treat null as "unknown" and must never fall back +/// to guessing which package was uploaded, because the identity is used to write availability and +/// pre-download dates and writing them against the wrong package is worse than not writing them at all. +/// +/// This project does not enable nullable reference types, so the null case is stated here rather than +/// being expressed as an annotation. +/// +internal sealed record Msixvc2ProcessResult(int ExitCode, string UploadedPackageId); diff --git a/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs b/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs index ec000a3f..793e2786 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2ProcessRunner.cs @@ -16,9 +16,16 @@ namespace PackageUploader.Application.Tools; /// internal sealed class Msixvc2ProcessRunner(ILogger logger) : IMsixvc2ProcessRunner { + /// + /// MakePkg.exe announces the package it is uploading with an info-level line of the form + /// "Package Id is <guid>". Verified against makepkg2.exe 2604.405.14000.0, where the line is printed + /// at default verbosity (no /v required) and before the content transfer begins. + /// + private const string PackageIdMarker = "Package Id is "; + private readonly ILogger _logger = logger ?? throw new ArgumentNullException(nameof(logger)); - public async Task RunAsync(string executablePath, string arguments, CancellationToken ct) + public async Task RunAsync(string executablePath, string arguments, CancellationToken ct) { using var process = new Process { @@ -41,11 +48,39 @@ public async Task RunAsync(string executablePath, string arguments, Cancell process.StartInfo.Environment[Msixvc2DelegationGuard.EnvironmentVariableName] = Msixvc2DelegationGuard.EnvironmentVariableValue; + // Kept local rather than on the instance so that concurrent or repeated runs cannot observe one + // another's package identity. Both output streams are scanned, so the lock is load-bearing. + var packageIdLock = new object(); + string uploadedPackageId = null; + var packageIdAmbiguous = false; + + void CapturePackageId(string line) + { + if (!TryParsePackageId(line, out var packageId)) + { + return; + } + + lock (packageIdLock) + { + if (uploadedPackageId is null) + { + uploadedPackageId = packageId; + } + else if (!string.Equals(uploadedPackageId, packageId, StringComparison.OrdinalIgnoreCase)) + { + // Two different identities means we cannot say which package the dates belong to. + packageIdAmbiguous = true; + } + } + } + process.OutputDataReceived += (_, e) => { if (!string.IsNullOrEmpty(e.Data)) { _logger.LogInformation("[MakePkg] {Data}", e.Data); + CapturePackageId(e.Data); } }; @@ -54,6 +89,7 @@ public async Task RunAsync(string executablePath, string arguments, Cancell if (!string.IsNullOrEmpty(e.Data)) { _logger.LogWarning("[MakePkg] {Data}", e.Data); + CapturePackageId(e.Data); } }; @@ -71,7 +107,47 @@ public async Task RunAsync(string executablePath, string arguments, Cancell throw; } - return process.ExitCode; + if (packageIdAmbiguous) + { + _logger.LogWarning( + "MakePkg.exe reported more than one package id, so the uploaded package cannot be identified."); + + uploadedPackageId = null; + } + + return new Msixvc2ProcessResult(process.ExitCode, uploadedPackageId); + } + + /// + /// Pulls the package identity out of a MakePkg.exe output line. Deliberately strict: the value must be + /// a bare GUID in the canonical form, because a loose match risks reporting an identity that is not a + /// package and having availability dates written against it. + /// + private static bool TryParsePackageId(string line, out string packageId) + { + packageId = null; + + var markerIndex = line.IndexOf(PackageIdMarker, StringComparison.OrdinalIgnoreCase); + if (markerIndex < 0) + { + return false; + } + + var candidate = line[(markerIndex + PackageIdMarker.Length)..].Trim(); + + var separatorIndex = candidate.IndexOf(' '); + if (separatorIndex >= 0) + { + candidate = candidate[..separatorIndex]; + } + + if (!Guid.TryParseExact(candidate, "D", out var parsed)) + { + return false; + } + + packageId = parsed.ToString(); + return true; } private void KillProcess(Process process) diff --git a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs index 7b27d662..6a60f988 100644 --- a/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs +++ b/src/PackageUploader.Application/Tools/Msixvc2UploadArgumentBuilder.cs @@ -42,6 +42,11 @@ namespace PackageUploader.Application.Tools; /// /// Options that MakePkg.exe has no equivalent for are either warned about and ignored (when ignoring them /// cannot change the outcome) or cause a (when it could). +/// +/// Note that availabilityDate/preDownloadDate are deliberately NOT handled here. MakePkg.exe +/// has no flag for them, but they are still honoured: the operation applies them through ingestion after the +/// upload, using the package identity MakePkg.exe reports. This builder must therefore leave them alone +/// rather than treat them as unsupported. /// internal static class Msixvc2UploadArgumentBuilder { @@ -264,23 +269,6 @@ private static void ValidateUnsupportedOptions( logger.LogWarning( "'deltaUpload' is not used for MSIXVC2 uploads and will be ignored; MakePkg.exe decides its own chunk reuse strategy."); } - - // Availability/pre-download dates are applied today by a post-upload SetXvcConfigurationAsync call, - // which needs the specific GamePackage that was just uploaded. MakePkg.exe does not report that - // identity back, and guessing which package it created could reconfigure the wrong package. - if (config.AvailabilityDate?.IsEnabled == true) - { - throw new Msixvc2UnsupportedOptionException( - "'availabilityDate' cannot be applied during an MSIXVC2 upload because MakePkg.exe does not report which package it uploaded. " + - "Remove it from this config and set the availability date in Partner Center, or with a separate PackageUploader invocation, after the upload completes."); - } - - if (config.PreDownloadDate?.IsEnabled == true) - { - throw new Msixvc2UnsupportedOptionException( - "'preDownloadDate' cannot be applied during an MSIXVC2 upload because MakePkg.exe does not report which package it uploaded. " + - "Remove it from this config and set the pre-download date in Partner Center, or with a separate PackageUploader invocation, after the upload completes."); - } } /// From 3469b287f4ff3c6338b77534dfdcaad687d63d1e Mon Sep 17 00:00:00 2001 From: "Jason Williams (Xbox)" <19478513+WilliamsJason@users.noreply.github.com> Date: Mon, 17 Aug 2026 10:54:56 -0700 Subject: [PATCH 10/10] Replace the MSIXVC2 capability placeholder with the shared tool resolver CHANGE 1 has landed, so IMsixvc2UploadToolProvider now sits over PackageUploader.ClientApi.Tools.IMsixvc2ToolResolver instead of the always-available placeholder. Msixvc2CapabilityPlaceholder is deleted and both TODO(GDK-release) markers are gone. The adapter resolves with no path hints, i.e. pure self-discovery. The UI passes already-resolved paths because it has its own file pickers; the CLI has no such input. It honors the provider contract that the compiler cannot express here, this project having nullable reference types off: - Resolve() returning null maps to IsAvailable false and a null ExecutablePath, so "no capable tool" stays the clean, actionable error UploadXvcPackageOperation already reports. - Resolution happens exactly once and both members are served from that one result. The resolver deliberately does not cache and re-probes by launching a candidate executable on every call, so a two-call adapter would probe repeatedly per upload and could disagree with itself between reads. UploadMsixvc2PackageAsync reads the members three times, so this is not hypothetical. - Nothing escapes as an exception. The resolver is documented as never throwing; the catch is defense in depth, degrading to unavailable. Registered scoped rather than singleton so each operation gets a fresh resolution instead of one cached for the life of the process. Verified against the real GDK rather than only mocks: the legacy MakePkg.exe in the GDK bin directory fails the uploadsource probe, the resolver falls back to makepkg2.exe alongside it, and a live upload to branch JaswillTest completed and applied its availability date. The probe pair appears exactly once in that run, confirming the single resolution end to end. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Tools/Msixvc2ToolResolverAdapterTest.cs | 137 ++++++++++++++++++ .../Extensions/HostExtensions.cs | 8 +- .../Tools/Msixvc2CapabilityPlaceholder.cs | 28 ---- .../Tools/Msixvc2ToolResolverAdapter.cs | 82 +++++++++++ 4 files changed, 224 insertions(+), 31 deletions(-) create mode 100644 src/PackageUploader.Application.Test/Tools/Msixvc2ToolResolverAdapterTest.cs delete mode 100644 src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs create mode 100644 src/PackageUploader.Application/Tools/Msixvc2ToolResolverAdapter.cs diff --git a/src/PackageUploader.Application.Test/Tools/Msixvc2ToolResolverAdapterTest.cs b/src/PackageUploader.Application.Test/Tools/Msixvc2ToolResolverAdapterTest.cs new file mode 100644 index 00000000..fd0d9738 --- /dev/null +++ b/src/PackageUploader.Application.Test/Tools/Msixvc2ToolResolverAdapterTest.cs @@ -0,0 +1,137 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; +using PackageUploader.Application.Tools; +using PackageUploader.ClientApi.Tools; +using System; + +namespace PackageUploader.Application.Test.Tools; + +/// +/// Covers the adapter that replaced the pre-rebase capability placeholder. The placeholder reported +/// the capability as unconditionally available, so the "unavailable" branch of +/// had only ever been exercised through a mock of the +/// interface itself. These tests exercise it through the real adapter. +/// +[TestClass] +public class Msixvc2ToolResolverAdapterTest +{ + private Mock _resolver = null!; + + [TestInitialize] + public void Initialize() + { + _resolver = new Mock(MockBehavior.Strict); + } + + private Msixvc2ToolResolverAdapter CreateAdapter() => + new(_resolver.Object, NullLogger.Instance); + + [TestMethod] + public void IsAvailable_WhenResolverReturnsNull_IsFalse() + { + _resolver.Setup(r => r.Resolve()).Returns((Msixvc2Tool)null!); + + var adapter = CreateAdapter(); + + Assert.IsFalse(adapter.IsAvailable); + } + + [TestMethod] + public void ExecutablePath_WhenResolverReturnsNull_IsNullOrEmpty() + { + _resolver.Setup(r => r.Resolve()).Returns((Msixvc2Tool)null!); + + var adapter = CreateAdapter(); + + Assert.IsTrue(string.IsNullOrEmpty(adapter.ExecutablePath)); + } + + [TestMethod] + public void Members_WhenResolverReturnsTool_ReportItAsAvailable() + { + _resolver.Setup(r => r.Resolve()).Returns(new Msixvc2Tool(@"C:\gdk\bin\MakePkg.exe", false)); + + var adapter = CreateAdapter(); + + Assert.IsTrue(adapter.IsAvailable); + Assert.AreEqual(@"C:\gdk\bin\MakePkg.exe", adapter.ExecutablePath); + } + + [TestMethod] + public void Members_WhenResolverReturnsMakePkg2Fallback_StillReportTheResolvedPath() + { + _resolver.Setup(r => r.Resolve()).Returns(new Msixvc2Tool(@"C:\gdk\bin\makepkg2.exe", true)); + + var adapter = CreateAdapter(); + + Assert.IsTrue(adapter.IsAvailable); + Assert.AreEqual(@"C:\gdk\bin\makepkg2.exe", adapter.ExecutablePath); + } + + /// + /// The resolver deliberately does not cache and re-probes by launching a candidate executable on + /// every call, so a two-call adapter would run that probe twice per upload. + /// + [TestMethod] + public void ReadingBothMembers_ResolvesExactlyOnce() + { + _resolver.Setup(r => r.Resolve()).Returns(new Msixvc2Tool(@"C:\gdk\bin\MakePkg.exe", false)); + + var adapter = CreateAdapter(); + + _ = adapter.IsAvailable; + _ = adapter.ExecutablePath; + _ = adapter.IsAvailable; + + _resolver.Verify(r => r.Resolve(), Times.Once); + } + + [TestMethod] + public void ReadingBothMembers_WhenUnavailable_StillResolvesExactlyOnce() + { + _resolver.Setup(r => r.Resolve()).Returns((Msixvc2Tool)null!); + + var adapter = CreateAdapter(); + + _ = adapter.IsAvailable; + _ = adapter.ExecutablePath; + + _resolver.Verify(r => r.Resolve(), Times.Once); + } + + /// + /// Self-discovery only. The CLI has no path hints to offer, unlike the UI which supplies paths + /// from its own file pickers. + /// + [TestMethod] + public void Adapter_UsesSelfDiscovery_NotThePathHintOverload() + { + _resolver.Setup(r => r.Resolve()).Returns(new Msixvc2Tool(@"C:\gdk\bin\MakePkg.exe", false)); + + var adapter = CreateAdapter(); + + _ = adapter.IsAvailable; + + _resolver.Verify(r => r.Resolve(It.IsAny(), It.IsAny()), Times.Never); + } + + /// + /// The resolver is documented as never throwing, but UploadXvcPackageOperation is only written to + /// handle "unavailable" as a clean error. An exception escaping the adapter would be a new failure + /// mode, so the adapter degrades to unavailable instead. + /// + [TestMethod] + public void Members_WhenResolverThrows_DegradeToUnavailableInsteadOfPropagating() + { + _resolver.Setup(r => r.Resolve()).Throws(new InvalidOperationException("probe exploded")); + + var adapter = CreateAdapter(); + + Assert.IsFalse(adapter.IsAvailable); + Assert.IsTrue(string.IsNullOrEmpty(adapter.ExecutablePath)); + } +} diff --git a/src/PackageUploader.Application/Extensions/HostExtensions.cs b/src/PackageUploader.Application/Extensions/HostExtensions.cs index 69d39279..0292cc59 100644 --- a/src/PackageUploader.Application/Extensions/HostExtensions.cs +++ b/src/PackageUploader.Application/Extensions/HostExtensions.cs @@ -11,6 +11,7 @@ using PackageUploader.Application.Tools; using PackageUploader.ClientApi; using PackageUploader.ClientApi.Client.Ingestion.TokenProvider.Models; +using PackageUploader.ClientApi.Tools; using PackageUploader.FileLogger; using System; using System.Collections.Generic; @@ -78,9 +79,10 @@ internal static HostApplicationBuilder ConfigureServices(this HostApplicationBui hostAppBuilder.Services.AddSingleton(); hostAppBuilder.Services.AddSingleton(); hostAppBuilder.Services.AddSingleton(); - // TODO(GDK-release): swap Msixvc2CapabilityPlaceholder for an adapter over - // PackageUploader.ClientApi.Tools.IMsixvc2ToolResolver once PR # merges. - hostAppBuilder.Services.AddSingleton(); + hostAppBuilder.Services.AddMsixvc2ToolResolver(); + // Scoped, not singleton: the adapter resolves once per instance, so a scoped lifetime gives + // each operation a fresh resolution instead of one cached for the life of the process. + hostAppBuilder.Services.AddScoped(); hostAppBuilder.Services .AddScoped() diff --git a/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs b/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs deleted file mode 100644 index 6c38f484..00000000 --- a/src/PackageUploader.Application/Tools/Msixvc2CapabilityPlaceholder.cs +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT License. - -namespace PackageUploader.Application.Tools; - -/// -/// PLACEHOLDER implementation of . -/// -/// TODO(GDK-release): replace with IMsixvc2ToolResolver from PackageUploader.ClientApi once PR #<change-1> merges. -/// That PR adds PackageUploader.ClientApi.Tools.IMsixvc2ToolResolver, which performs the real -/// MakePkg.exe (preferred) / makepkg2.exe (fallback) discovery and returns null when unavailable. -/// The rebase is intended to be a one-line change: re-point the IMsixvc2UploadToolProvider -/// registration in HostExtensions.ConfigureServices at an adapter over IMsixvc2ToolResolver. -/// Until then this placeholder reports the capability as always available and relies on -/// MakePkg.exe being resolvable from PATH. -/// -internal sealed class Msixvc2CapabilityPlaceholder : IMsixvc2UploadToolProvider -{ - private const string PlaceholderExecutable = "MakePkg.exe"; - - // TODO(GDK-release): replace with IMsixvc2ToolResolver.IsMsixvc2Supported(). - public bool IsAvailable => true; - - // TODO(GDK-release): replace with IMsixvc2ToolResolver.Resolve()?.ExecutablePath. - // The replacement must return null/empty (never throw) when Resolve() finds no capable tool, - // and must share a single Resolve() call with IsAvailable. See IMsixvc2UploadToolProvider's contract. - public string ExecutablePath => PlaceholderExecutable; -} diff --git a/src/PackageUploader.Application/Tools/Msixvc2ToolResolverAdapter.cs b/src/PackageUploader.Application/Tools/Msixvc2ToolResolverAdapter.cs new file mode 100644 index 00000000..27a2cfba --- /dev/null +++ b/src/PackageUploader.Application/Tools/Msixvc2ToolResolverAdapter.cs @@ -0,0 +1,82 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Logging; +using PackageUploader.ClientApi.Tools; +using System; + +namespace PackageUploader.Application.Tools; + +/// +/// Adapts from PackageUploader.ClientApi to this project's +/// . +/// +/// Resolution is performed with no path hints, i.e. pure self-discovery (application directory, +/// current directory, the installed GDK, then PATH). The UI passes already-resolved paths because it +/// has its own file pickers; the CLI has no such input and deliberately relies on self-discovery. +/// +/// +/// +/// The underlying resolver intentionally does not cache: every Resolve() call re-probes a +/// candidate executable by launching it. This adapter therefore resolves EXACTLY ONCE and serves both +/// members from that single result, which satisfies the "single underlying resolution" clause of +/// 's contract. Reading both members must not launch the probe +/// twice, and must not be able to report an available tool alongside a null path if the environment +/// changes mid-operation. +/// +/// +/// This type is registered per-scope so that each operation gets a fresh resolution rather than one +/// cached for the lifetime of the process. +/// +/// +internal sealed class Msixvc2ToolResolverAdapter : IMsixvc2UploadToolProvider +{ + private readonly IMsixvc2ToolResolver _resolver; + private readonly ILogger _logger; + + private bool _resolved; + private Msixvc2Tool _tool; + + public Msixvc2ToolResolverAdapter(IMsixvc2ToolResolver resolver, ILogger logger) + { + _resolver = resolver ?? throw new ArgumentNullException(nameof(resolver)); + _logger = logger; + } + + /// + /// True when produced a tool. A null result means + /// "no MSIXVC2-capable tool is installed", which is an ordinary outcome, not an error. + /// + public bool IsAvailable => ResolveOnce() is not null; + + /// + /// The resolved executable path, or null when no capable tool was found. Always consistent + /// with because both read the same cached resolution. + /// + public string ExecutablePath => ResolveOnce()?.ExecutablePath; + + private Msixvc2Tool ResolveOnce() + { + if (_resolved) + { + return _tool; + } + + // The resolver is documented as never throwing, returning null for "nothing capable found". + // This catch is defense in depth only: UploadXvcPackageOperation is written to report an + // unavailable tool as a clean, actionable message, so an exception escaping here would be a + // new failure mode it cannot handle. Degrade to "unavailable" instead. + try + { + _tool = _resolver.Resolve(); + } + catch (Exception ex) + { + _logger?.LogDebug(ex, "MSIXVC2 tool resolution threw; treating MSIXVC2 as unavailable."); + _tool = null; + } + + _resolved = true; + return _tool; + } +}