Add MSIXVC2 upload support to PackageUploader.exe via MakePkg.exe - #135
Merged
Jason Williams (WilliamsJason) merged 14 commits intoAug 31, 2026
Merged
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Jason Williams (WilliamsJason)
marked this pull request as draft
August 12, 2026 20:40
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>
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>
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>
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>
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 <GDKInstallPath>\bin, so no NuGet package is required. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This branch introduced the repository's only six `#nullable enable` directives. The repo does use nullable reference types, but always project-wide via `<Nullable>enable</Nullable>` 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<T> 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>
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>
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>
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 <guid>" 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 <guid>" 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>
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>
Jason Williams (WilliamsJason)
force-pushed
the
jaswill-microsoft-msixvc2-cli-upload
branch
from
August 17, 2026 17:55
554b7d2 to
3469b28
Compare
Jason Williams (WilliamsJason)
marked this pull request as ready for review
August 17, 2026 22:16
Jon Caruana (joncarmsft)
approved these changes
Aug 25, 2026
Correct the EKB/submission-validator rationale, fix the delegation-guard comment for the October MakePkg.exe rename, reject SODB assets that MakePkg.exe cannot upload, hedge the fall-through warnings, and remove the overstated credential-exposure warning. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
MakePkg no longer shells back out to PackageUploader.exe for XVC1 uploads, and the same release adds the full parity option surface. Both facts change this feature substantially. Capability probe replaces the recursion guard --------------------------------------------- The whole cycle-detection subsystem is deleted: Msixvc2DelegationGuard, IParentProcessProvider, ParentProcessProvider and their tests. In its place UploadXvcPackageOperation runs `makepkg supports xvc1upload` against the resolved executable before launching it. One probe answers both questions at once. The MakePkg release that started advertising `xvc1upload` is the same one that stopped invoking PackageUploader.exe, so a tool that passes the probe both can perform the upload and will not call back into us. A tool that fails it is never launched, so no cycle is reachable by construction. Verified against the real published binary (2610.0.0.0), not inferred: `supports xvc1upload` exits 0 and `supports bogusfeature` exits 6. Parity options are now forwarded instead of rejected ---------------------------------------------------- - Availability and pre-download dates travel on the command line, and MakePkg applies the schedule. PackageUploader no longer writes dates through ingestion after an MSIXVC2 upload, so the post-upload package lookup and the `Package Id is <guid>` stdout scraping are both gone. The GamePackageDate tri-state maps onto the flag pairs: enabled sets, disabled sends the matching clear flag, absent emits nothing. Without the clear flags "clear the date" would be indistinguishable from "leave it alone". - `/sodb` exists, so the SODB path is forwarded from any location rather than rejected. - All three certificate selectors are supported. MakePkg requires exactly one of /certpath, /certthumbprint and /certsubject, so that rule is enforced up front with a message naming our own config keys. /certstore and /certlocation accompany only the store selectors; /certpassword accompanies only the file selector. - /pd takes the package file, not its directory. A directory is ambiguous the moment it holds more than one package. - /delta is deliberately not emitted. MakePkg is removing the flag and decides delta status itself; `deltaUpload` warns instead. - /disclayout is rejected: MakePkg refuses it for every non-XVC1 format and has no MSIXVC2 disc-layout asset upload. Loose game content is refused explicitly ---------------------------------------- The MSIXVC2 pack-and-upload flow belongs to MakePkg end to end, and PackageUploader has no packaging step. A content directory or a MicrosoftGame.config now fails with a message that says so, instead of reaching the upload layer and producing "package file not found". Credential handling is unchanged in shape: the log-safe command line is built from a credential-free context rather than scrubbed after the fact, and /certpassword is redacted alongside /clientsecret. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
End-to-end testing against a release MakePkg.exe surfaced a bad error message: pointing packageFilePath at a loose content directory reported "The GameAssets field is required" instead of the operation's message explaining that the path is loose content and PackageUploader cannot build a package from it. Config validation runs before the operation, and loose content is not MSIXVC2, so it fell into the branch that demands EKB and submission-validator paths. Those assets only matter on the upload path PackageUploader drives itself; loose content is refused before reaching it, so requiring them told the user to supply an EKB for content that has no package to attach one to. Exempt it for the same reason MSIXVC2 is already exempt. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Review feedback caught the certificate path error naming AadAuthInfo:CertificatePath. No such key exists: AadAuthInfo and its subclasses declare only the store-based selectors, while CertificatePath and CertificatePassword live on ClientCertificateAuthInfo. The binding in HostExtensions already read the right section, so only the message was wrong -- and it sent users to edit a setting that would never be read. The README already documented the correct key. Derive the paths named in errors from the binding models with nameof instead of writing them out by hand, so a renamed property breaks the build rather than silently misdirecting the user. Verifying that turned up the same class of bug on ManagedIdentityFederated. The context read AadAuthInfo:ResourceId, a key no model declares, and the value was forwarded only when present. MakePkg.exe requires /resourceid for that method, so the option was silently dropped and the child process failed with its own message. PackageUploader's federated model has no resource id to map from -- it uses a user-assigned identity client id plus an application tenant and client id -- so the key remains a direct configuration read, but selecting the method without it now fails up front naming the key, consistent with how the other credential gaps are handled. Pin the certificate keys with a test that resolves each one against the type binding its section, so the assertion fails if either the message or the model moves. Restoring the original bug makes it fail. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
elahmed-microsoft
approved these changes
Aug 31, 2026
Jason Williams (WilliamsJason)
deleted the
jaswill-microsoft-msixvc2-cli-upload
branch
August 31, 2026 18:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds MSIXVC2 upload support to
PackageUploader.exe.UploadXvcPackagedetects the format of the filepackageFilePathpoints at. When it is an MSIXVC2 package, the upload is delegated toMakePkg.exe, which owns the MSIXVC2 upload protocol. There is no new operation name and no new config switch. The XVC1/MSIXVC1 path is untouched.When delegation happens
Both must hold, or the operation stops with an actionable error:
PackageFormatDetectorinPackageUploader.ClientApi).MakePkg.exereports the capability:makepkg supports xvc1uploadexits 0.The capability gate matters beyond version-checking. MakePkg gained the
xvc1uploadcapability in the same release that made it safe for PackageUploader to hand work to it, so a tool that fails the probe is never launched. Verified against the shipping binary (2610.0.0.0):supports xvc1upload→ 0,supports bogusfeature→ 6.Config to command line
Arguments are emitted in a fixed order so tests can assert the exact string:
availabilityDate/preDownloadDateGamePackageDatetri-state maps onto the flag pairs: enabled sets, disabled sends/clear...date, absent emits nothing. Without the clear flags, "clear the date" would be indistinguishable from "leave it alone".gameAssets.sodbFilePath/sodbfrom any location.gameAssetsEKB / SubVal / symbolsgameAssets.discLayoutFilePath/disclayoutfor every non-XVC1 format and has no MSIXVC2 disc-layout asset upload.deltaUploadminutesToWaitForProcessingproductId/storeidtakes.AppSecret/AppCertare aliased toClientSecret/ClientCertificate.Certificates
MakePkg requires exactly one of
/certpath,/certthumbprint,/certsubjectand fails a command line carrying more than one. That rule is enforced before launch instead, so the error names PackageUploader's own config keys and arrives before any work starts./certstoreand/certlocationare modifiers for store lookups only;/certpasswordaccompanies only the file selector.Details worth calling out
/pdtakes the package file, not its directory. A directory is ambiguous the moment it holds more than one package. Confirmed against the shipping binary's help.GamePackageDate(UTC, truncated to the hour) before the builder sees them, so the delegated path and the XVC1 path agree on the instant. Pinned by a test.Loose game content
PackageUploader uploads a built package and has no packaging step; for MSIXVC2 the pack-and-upload flow belongs to MakePkg end to end. A content directory or a
MicrosoftGame.configis refused up front with a message that says so, rather than reaching the upload layer and producingPackage file not found— true, but explaining nothing.Tests
PackageUploader.Application.TestPackageUploader.ClientApi.TestCovering: capability probe pass and fail; the probe never running for a non-MSIXVC2 package; exact argument strings; each unsupported option's error or warning; credential redaction for
/clientsecretand/certpassword; non-zero child exit propagation; cancellation killing the child; and loose-content rejection at both the detector and operation level.Dependency
Requires MakePkg changes that will not ship until the October 2026 GDK. Until then the capability probe reports the feature as unavailable, so an older MakePkg is never launched: an MSIXVC2 upload fails with a clear "install the latest GDK" error rather than attempting something that cannot succeed. Nothing on the XVC1/MSIXVC1 path is affected.